Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12253057:
Implement WebStorage API methods (Closed)
|
Created:
7 years, 10 months ago by boliu Modified:
7 years, 10 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionImplement WebStorage API methods
Most of the methods involves calling methods to QuotaManager
on the IO thread and translating the arguments between Java
and native code. Introduce AwQuotaManagerBridge to facilitate
this logic.
The Java AwQuotaManagerBridge is currently a singleton but
should be owned by AwBrowserContext when we have one. The
native one is owned by native AwBrowserContext. Java calls
the corresponding native AwBrowserContext to obtain the
pointer.
Introduced JniDependencyFactory interface used to create
native objects under native but is used or passed in
BrowserContext or ContentsBrowserClient.
Also added base::android::ToJavaLongArray to convert to
Java long arrays.
BUG=
Android only change. Ran through android bots.
NOTRY=true
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184139
Patch Set 1 #
Total comments: 1
Patch Set 2 : Implementation mostly done. #
Total comments: 46
Patch Set 3 : Leveling up base::Bind-foo. -150 lines. Implementation code ready for review. Still need real tests. #Patch Set 4 : Rearrange usage/quota #
Total comments: 34
Patch Set 5 : Address comments. Still don't look at tests. #
Total comments: 2
Patch Set 6 : Tests still work in progress :( #
Total comments: 18
Patch Set 7 : Real tests passing! #
Total comments: 1
Patch Set 8 : Added minimal comments #
Total comments: 14
Patch Set 9 : Address comments from joth. #
Total comments: 6
Patch Set 10 : Address Selim's comments. #
Total comments: 6
Patch Set 11 : Address nits #Messages
Total messages: 31 (0 generated)
Not ready for full review yet but breaking some new ground so uploading. Ideally this would depend on joth's java AwBrowserContext lands (https://codereview.chromium.org/12208099/) and have the java objects be associated directly with the native ones. Right now I'm having BrowserContext own QuotaManagerBridge in both cases, so there is no lifetime link between the two QuotaManagerBridges. I suppose it is possible for the native side to be deleted while java side is waiting to be gc-ed although this probably won't happen in practice. Either way probably better to have java own native like the other classes to avoid problems. Another thing is dealing with java/native callbacks. Right now I'm saving the java callbacks in a int->callback map and passing the int to native side. Also created a native_factory class to create objects under native that's used under browser. So much boilerplate code without writing a single line of actual logic...
https://codereview.chromium.org/12253057/diff/1/android_webview/browser/aw_qu... File android_webview/browser/aw_quota_permission_context.cc (right): https://codereview.chromium.org/12253057/diff/1/android_webview/browser/aw_qu... android_webview/browser/aw_quota_permission_context.cc:29: if (new_quota > 10 * 1024 * 1024) { We shouldn't have persistent storage in WebView now. It is only used by File System API which we don't currently support (as the old WebView wasn't supporting it). See https://developers.google.com/chrome/whitepapers/storage
Pretty epic patch...thread topping, callbacks, native->java conversions. Basic implementation done, still need cleanups, add unit tests for jni_array, and real java tests.
https://codereview.chromium.org/12253057/diff/5001/android_webview/browser/aw... File android_webview/browser/aw_quota_permission_context.cc (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/browser/aw... android_webview/browser/aw_quota_permission_context.cc:26: callback.Run(QUOTA_PERMISSION_RESPONSE_DISALLOW); This path is only used for PERSISTENT storage, so just straight disallow. Still have to check how temporary quota works.
https://codereview.chromium.org/12253057/diff/5001/android_webview/browser/aw... File android_webview/browser/aw_quota_permission_context.cc (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/browser/aw... android_webview/browser/aw_quota_permission_context.cc:26: callback.Run(QUOTA_PERMISSION_RESPONSE_DISALLOW); On 2013/02/16 03:55:15, boliu wrote: > This path is only used for PERSISTENT storage, so just straight disallow. Still > have to check how temporary quota works. maybe add a comment later on with that justification? https://codereview.chromium.org/12253057/diff/5001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:28: public static AwQuotaManagerBridge getInstance() { maybe document/assert that this is only used from one thread, otherwise it's not threadsafe. https://codereview.chromium.org/12253057/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:53: private Map<Integer, ValueCallback<Origins>> mPendingGetOriginCallbacks; I've been ranting about this for a while now. We should have a "legit" (ie. not ghetto) way of linking native and Java callbacks together. You feel like doing that? I'll gladly help you out with the design and reviews. Alternatively I could try and write that code (and get you to review it :P). https://codereview.chromium.org/12253057/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:87: assert !mPendingGetQuotaForOriginCallbacks.containsKey(callbackId); maybe have the assert in getNextId? https://codereview.chromium.org/12253057/diff/5001/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java:15: public class AwQuotaManagerBridgeTest extends AndroidWebViewTestBase { ah, I assume you need the browser process to be set up in order to run this test? otherwise it could be a plain InstrumentationTest... https://codereview.chromium.org/12253057/diff/5001/android_webview/lib/aw_bro... File android_webview/lib/aw_browser_dependency_factory_impl.cc (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/lib/aw_bro... android_webview/lib/aw_browser_dependency_factory_impl.cc:7: #include "android_webview/browser/aw_browser_context.h" needed? https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... File android_webview/native/aw_quota_manager_bridge.cc (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:30: class GetOriginsIOThreadSwitcher this seems more like an OriginsUsageAndQuotaGetter (the fact that it has to do with threads should be completely encapsulated IMO) https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:34: const base::WeakPtr<AwQuotaManagerBridge>& bridge, IMO this dude should take a on_results_obtained base::Callback (and jcallback_id should simply be bound into that callback) and a QuotaManager directly as parameters. I guess the best interface would be one static method that takes the previously mentioned parameters, create the object and post to the IO thread. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:37: void GetOriginsOnIOThread(); this should be an internal detail. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:74: AddRef(); // Matched in DoneOnUIThread. this calls for the "The more you know" meme, but did you know that base::Bind will automagically addref and relase for you? From the usage of this guy it seems you could do without the addref/release pair (although it does _seem_ safer to have them in). https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:82: base::Bind(&GetOriginsIOThreadSwitcher::GetOriginsCallback, nit: OnOriginsObtained ? https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:98: base::Bind(&GetOriginsIOThreadSwitcher::AddToData, OnUsageAndQuotaObtained? https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:156: void IOThreadSwitcher::DeleteAllDataOnIOThread() { DeleteAllOriginData...? https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:164: void IOThreadSwitcher::DeleteOriginOnIOThread(const GURL& origin) { DeleteOriginData... ? https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:179: base::Bind(&IOThreadSwitcher::GetQuotaForOriginCallback, OnQuotaObtained? https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:190: base::Bind(&IOThreadSwitcher::GetUsageForOriginCallback, OnUsageObtained? https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:195: void IOThreadSwitcher::DeleteOrigins( DeleteOriginDataOnIOThread ? https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:229: void IOThreadSwitcher::GetUsageForOriginCallback( seems like you might be better off to have one path for getting both the quota and usage and then simply ignore one of the values Java-side. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:246: quota)); s/usage/quota/ see? you really do need to merge the two paths, otherwise mistakes like this will pop up in this code sooner or later. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:267: content::ContentBrowserClient* cbc = content::GetContentClient()->browser(); uh.. maybe just browserClient? https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:270: AwContentBrowserClient* acbc = awBrowserClient? https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:310: BrowserThread::PostTask( IMO this means that the encapsulation provided by the IOThreadSwitcher failed. I think it would be better to call methods on the IOThreadSwitcher and have the IOThreadSwitcher do the required Post calls. That way you have all the threading calls in one place. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... File android_webview/native/aw_quota_manager_bridge.h (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.h:36: class IOThreadSwitcher : public base::RefCountedThreadSafe<IOThreadSwitcher> { this is a bit nasty. I get that you need to spill out the implementation because you're using scoped_refptr but maybe, as a form of damage control, you could only declare the public interface in the header (and have an Impl class in the .cc file)? Looking at this a bit more I don't quite see why do you need a scoped_refptr to this switcher but end newing up the other switcher on the spot - please be consistent. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.h:38: IOThreadSwitcher(const base::WeakPtr<AwQuotaManagerBridge>& bridge); to be honest I think this class and the other switcher deserve their own .h and .cc file and a slighlty better interface. It seems to me you're trying to create a subset of the QuotaManager interface that works on the UI thread. Maybe merge the two switchers and call them QuotaManagerThreadAdapter (or something) and have the interface mimic the QuotaManager more closely? You want to aim for the QuotaManagerBridge to not have to do any Post calls. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.h:40: void DeleteAllDataOnIOThread(); Looks like all of the methods are OnIOThread, maybe ditch the suffix since the class name sort of implies this happens on the IO Thread anyway.
Posting some replies without new patch/code. https://codereview.chromium.org/12253057/diff/5001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:87: assert !mPendingGetQuotaForOriginCallbacks.containsKey(callbackId); On 2013/02/19 11:37:13, Martin Kosiba wrote: > maybe have the assert in getNextId? There are 3 different maps... https://codereview.chromium.org/12253057/diff/5001/android_webview/javatests/... File android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/javatests/... android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java:15: public class AwQuotaManagerBridgeTest extends AndroidWebViewTestBase { On 2013/02/19 11:37:13, Martin Kosiba wrote: > ah, I assume you need the browser process to be set up in order to run this > test? otherwise it could be a plain InstrumentationTest... I'm not sure actually but I don't think so currently. I do plan on adding actual tests that loads javascript to test the whole thing though https://codereview.chromium.org/12253057/diff/5001/android_webview/lib/aw_bro... File android_webview/lib/aw_browser_dependency_factory_impl.cc (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/lib/aw_bro... android_webview/lib/aw_browser_dependency_factory_impl.cc:7: #include "android_webview/browser/aw_browser_context.h" On 2013/02/19 11:37:13, Martin Kosiba wrote: > needed? Yes because it's removed from aw_content_browser_client.h https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... File android_webview/native/aw_quota_manager_bridge.cc (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:74: AddRef(); // Matched in DoneOnUIThread. On 2013/02/19 11:37:13, Martin Kosiba wrote: > this calls for the "The more you know" meme, but did you know that base::Bind > will automagically addref and relase for you? > From the usage of this guy it seems you could do without the addref/release pair > (although it does _seem_ safer to have them in). I knew that...I just have to convince myself this object won't get deleted accidentally hopping between callbacks. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... File android_webview/native/aw_quota_manager_bridge.h (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.h:36: class IOThreadSwitcher : public base::RefCountedThreadSafe<IOThreadSwitcher> { On 2013/02/19 11:37:13, Martin Kosiba wrote: > this is a bit nasty. I get that you need to spill out the implementation because > you're using scoped_refptr but maybe, as a form of damage control, you could > only declare the public interface in the header (and have an Impl class in the > .cc file)? > > Looking at this a bit more I don't quite see why do you need a scoped_refptr to > this switcher but end newing up the other switcher on the spot - please be > consistent. Me and my micro/premature optimizations. I thought all the calls would map directly to one function in QuotaManager, so there'd be no state so I though, why not just new up one of these which lead to this. Then realized can't use QuotaManager::GetUsageInfo since it doesn't return quota info so GetOrigins will need get a list of origins and query each individually, so need to remember state, and thus the other switcher. Also switcher is a horrible name... I'll think of something. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.h:38: IOThreadSwitcher(const base::WeakPtr<AwQuotaManagerBridge>& bridge); On 2013/02/19 11:37:13, Martin Kosiba wrote: > to be honest I think this class and the other switcher deserve their own .h and > .cc file and a slighlty better interface. It seems to me you're trying to create > a subset of the QuotaManager interface that works on the UI thread. Maybe merge > the two switchers and call them QuotaManagerThreadAdapter (or something) and > have the interface mimic the QuotaManager more closely? You want to aim for the > QuotaManagerBridge to not have to do any Post calls. Just found base::PostTaskAndReplyWithResult. There is also media::base::BindToLoop which I wish is in base right now...
Implementation code ready for review. https://codereview.chromium.org/12253057/diff/5001/android_webview/browser/aw... File android_webview/browser/aw_quota_permission_context.cc (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/browser/aw... android_webview/browser/aw_quota_permission_context.cc:26: callback.Run(QUOTA_PERMISSION_RESPONSE_DISALLOW); On 2013/02/19 11:37:13, Martin Kosiba wrote: > On 2013/02/16 03:55:15, boliu wrote: > > This path is only used for PERSISTENT storage, so just straight disallow. > Still > > have to check how temporary quota works. > > maybe add a comment later on with that justification? Done. https://codereview.chromium.org/12253057/diff/5001/android_webview/java/src/o... File android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/java/src/o... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:28: public static AwQuotaManagerBridge getInstance() { On 2013/02/19 11:37:13, Martin Kosiba wrote: > maybe document/assert that this is only used from one thread, otherwise it's not > threadsafe. Done. Copied from android so not sure if form is correct. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... File android_webview/native/aw_quota_manager_bridge.cc (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:30: class GetOriginsIOThreadSwitcher On 2013/02/19 11:37:13, Martin Kosiba wrote: > this seems more like an OriginsUsageAndQuotaGetter (the fact that it has to do > with threads should be completely encapsulated IMO) Renamed to GetOriginsTask. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:34: const base::WeakPtr<AwQuotaManagerBridge>& bridge, On 2013/02/19 11:37:13, Martin Kosiba wrote: > IMO this dude should take a on_results_obtained base::Callback (and jcallback_id > should simply be bound into that callback) and a QuotaManager directly as > parameters. > > I guess the best interface would be one static method that takes the previously > mentioned parameters, create the object and post to the IO thread. Did the bind jcallbakc_id into Callback thing. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:37: void GetOriginsOnIOThread(); On 2013/02/19 11:37:13, Martin Kosiba wrote: > this should be an internal detail. Renamed this to Run. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:74: AddRef(); // Matched in DoneOnUIThread. On 2013/02/19 17:03:21, boliu wrote: > On 2013/02/19 11:37:13, Martin Kosiba wrote: > > this calls for the "The more you know" meme, but did you know that base::Bind > > will automagically addref and relase for you? > > From the usage of this guy it seems you could do without the addref/release > pair > > (although it does _seem_ safer to have them in). > > I knew that...I just have to convince myself this object won't get deleted > accidentally hopping between callbacks. I'm convinced. Done :) https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:82: base::Bind(&GetOriginsIOThreadSwitcher::GetOriginsCallback, On 2013/02/19 11:37:13, Martin Kosiba wrote: > nit: OnOriginsObtained ? Done. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:98: base::Bind(&GetOriginsIOThreadSwitcher::AddToData, On 2013/02/19 11:37:13, Martin Kosiba wrote: > OnUsageAndQuotaObtained? Done. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:229: void IOThreadSwitcher::GetUsageForOriginCallback( On 2013/02/19 11:37:13, Martin Kosiba wrote: > seems like you might be better off to have one path for getting both the quota > and usage and then simply ignore one of the values Java-side. Done. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:246: quota)); On 2013/02/19 11:37:13, Martin Kosiba wrote: > s/usage/quota/ > > see? you really do need to merge the two paths, otherwise mistakes like this > will pop up in this code sooner or later. Since both are int64, so it's still easy to mess them up. I've tried to make it usage first, quota second everywhere. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:267: content::ContentBrowserClient* cbc = content::GetContentClient()->browser(); On 2013/02/19 11:37:13, Martin Kosiba wrote: > uh.. maybe just browserClient? Done. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:270: AwContentBrowserClient* acbc = On 2013/02/19 11:37:13, Martin Kosiba wrote: > awBrowserClient? Done. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.cc:310: BrowserThread::PostTask( On 2013/02/19 11:37:13, Martin Kosiba wrote: > IMO this means that the encapsulation provided by the IOThreadSwitcher failed. I > think it would be better to call methods on the IOThreadSwitcher and have the > IOThreadSwitcher do the required Post calls. That way you have all the threading > calls in one place. The old IOThreadSwitcher was never meant to encapsulate anything, just that it needs something that's refcoutnedthreadsafe to hop between threads in callbacks. https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... File android_webview/native/aw_quota_manager_bridge.h (right): https://codereview.chromium.org/12253057/diff/5001/android_webview/native/aw_... android_webview/native/aw_quota_manager_bridge.h:36: class IOThreadSwitcher : public base::RefCountedThreadSafe<IOThreadSwitcher> { On 2013/02/19 17:03:21, boliu wrote: > On 2013/02/19 11:37:13, Martin Kosiba wrote: > > this is a bit nasty. I get that you need to spill out the implementation > because > > you're using scoped_refptr but maybe, as a form of damage control, you could > > only declare the public interface in the header (and have an Impl class in the > > .cc file)? > > > > Looking at this a bit more I don't quite see why do you need a scoped_refptr > to > > this switcher but end newing up the other switcher on the spot - please be > > consistent. > > Me and my micro/premature optimizations. > > I thought all the calls would map directly to one function in QuotaManager, so > there'd be no state so I though, why not just new up one of these which lead to > this. Then realized can't use QuotaManager::GetUsageInfo since it doesn't return > quota info so GetOrigins will need get a list of origins and query each > individually, so need to remember state, and thus the other switcher. > > Also switcher is a horrible name... > > I'll think of something. Through the magic of base::Bind, this class is gone! https://codereview.chromium.org/12253057/diff/17001/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:110: int callbackId, boolean isQuota, long usage, long quota) { I've tried to follow usage first, quota second throughout the code so they don't get mixed up https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:29: namespace { The anonymous namespace is split into 3 pieces in this file to kind of group code logically. https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:31: class GetOriginsTask : public base::RefCountedThreadSafe<GetOriginsTask> { This could be moved to a separate h/cc file, but should sit inside another internal namespace. https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:134: void GetOriginsTask::DoneOnUIThread() { This and RunGetUsageAndQuotaCallback below can be removed if media::base::BindToLoop is moved to base. https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:189: QuotaClient::kAppcache | Mikhail: You've implemented some app cache methods. Is it cleared in a different path? Should the quota here include app cache as well (If not, then need to rewrite the QuotaManager api to add this flag, if possible...) Joth: Know if this will create a static initializer? I've never disassembled anything so wouldn't know where to check. https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.h (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.h:51: scoped_refptr<quota::QuotaManager> GetQuotaManager() const; I'm not sure if base::Bind will take something as refcounted, so I've been conservative and declared things as scoped_refptr https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.h:53: void GetOriginsCallbackImpl( Better name suggestion welcome..
https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/a... File android_webview/browser/aw_quota_manager_bridge.h (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/a... android_webview/browser/aw_quota_manager_bridge.h:10: // Empty super class so this can be owned by AwBrowserContext. I don't understand the usage of "super" here. Does this class act as a superclass for other classes? I didn't see any. https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/a... File android_webview/browser/aw_quota_permission_context.cc (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/a... android_webview/browser/aw_quota_permission_context.cc:26: // Android WebView only uses quota::kStorageTypeTemporary tyep of storage nit: "tyep" https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:189: QuotaClient::kAppcache | On 2013/02/20 00:53:51, boliu wrote: > Mikhail: You've implemented some app cache methods. Is it cleared in a different > path? Should the quota here include app cache as well (If not, then need to > rewrite the QuotaManager api to add this flag, if possible...) > I wasn't doing anything for clearing Appcache. It should be handled together with other OWP storage types, so your code is correct. In the legacy implementation, Appcache should be cleared by WebStorage.deleteOrigin. > Joth: Know if this will create a static initializer? I've never disassembled > anything so wouldn't know where to check. Sorry for intervening. The values of these constants are known at compile time, so this shouldn't create static a initializer. See https://docs.google.com/a/google.com/document/d/1Zk9JZR3z1L7LMXbd05tj04rClYcu... [As a sanity check, you can try creating an enum with this value, and the code just wouldn't compile if the operands values can't be determined at compile time]
This one is looking much much better! Thanks for all the improvements. https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/a... File android_webview/browser/aw_quota_manager_bridge.h (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/a... android_webview/browser/aw_quota_manager_bridge.h:10: // Empty super class so this can be owned by AwBrowserContext. On 2013/02/20 09:29:03, Mikhail Naganov (Chromium) wrote: > I don't understand the usage of "super" here. Does this class act as a > superclass for other classes? I didn't see any. probably "base class" is a better name. I'm assuming the idea is that you need the public destructor for AwBrowserContext to be able to destroy the bridge. Maybe add that to the comment so it's super-obvious :) https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:179: QuotaManager* quota_manager = storage_partition->GetQuotaManager(); This bit me a couple of times so just double-checking with you. Normally, if the object needs to be interacted with on the IO thread, you need to get the instance on the IO thread too. I'm assuming that storage_partition is UI-thread safe and the only thing it does with the QuotaManager is hold a pointer to it (and create it? But then QuotaManager would be constructed on the UI thread...) https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:194: void DeleteOrigins( nit2: it would also be nice if the name indicated it's "dedicated" for being invoked from the AwQuotaManagerBridgeImpl::DeleteAllData method. DeleteOriginDataWorker or DeleteAllOriginDataWorker? https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:212: void AwQuotaManagerBridgeImpl::DeleteAllData(JNIEnv* env, jobject object) { nit: still think this should match the quota manager API more closely (DeleteOriginData or DeleteAllOriginData). https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:225: void AwQuotaManagerBridgeImpl::DeleteOrigin( same nit: DeleteOriginData https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.h (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.h:51: scoped_refptr<quota::QuotaManager> GetQuotaManager() const; On 2013/02/20 00:53:51, boliu wrote: > I'm not sure if base::Bind will take something as refcounted, so I've been > conservative and declared things as scoped_refptr it works correctly for a pointer to anything that has AddRef and Release methods (base::Bind uses template magic to figure this out) https://codereview.chromium.org/12253057/diff/17001/base/android/jni_array.cc File base/android/jni_array.cc (right): https://codereview.chromium.org/12253057/diff/17001/base/android/jni_array.cc... base/android/jni_array.cc:29: JNIEnv* env, const int64* longs, size_t len) { I know the function above uses a raw C-style array but std::vector is just as efficient, guaranteed to take up a continuous block of memory (so memcpy-ing it is OK) and you don't risk mis-specifying the len. Also, it seems all ToJavaLongArray callsites use a vector. You probably should add a wrapper that takes in a vector and calls this method to do the actual work. https://codereview.chromium.org/12253057/diff/17001/base/android/jni_array.cc... base/android/jni_array.cc:35: memcpy(elements, longs, len * sizeof(*longs)); I can't remember what the exact issue was but one of the C++ books I read recommended using the type (int64) rather than the variable in this case. I'd say that it's OK to hardcode the type since the method does explicitly deal with longs (aka int64s) so the chances that you change the type of *longs and forget to update the code are low.
https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:187: int ALL_CLIENT_MASK = QuotaClient::kFileSystem | you should make it const. also, I think the kFoo style is preferred (modulo where we're sharing constant across c++ & java) so I'd be tempted to use that here, given the individual consts are kStyle. although, I see kAllClientsMask already declared in https://code.google.com/p/chromium/codesearch#chromium/src/webkit/quota/quota... why not using that? as you have it, you'd need to remember to update this too. (is that this intent? if so, should probably comment it) https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:189: QuotaClient::kAppcache | On 2013/02/20 09:29:03, Mikhail Naganov (Chromium) wrote: > On 2013/02/20 00:53:51, boliu wrote: > > Mikhail: You've implemented some app cache methods. Is it cleared in a > different > > path? Should the quota here include app cache as well (If not, then need to > > rewrite the QuotaManager api to add this flag, if possible...) > > > > I wasn't doing anything for clearing Appcache. It should be handled together > with other OWP storage types, so your code is correct. In the legacy > implementation, Appcache should be cleared by WebStorage.deleteOrigin. > > > Joth: Know if this will create a static initializer? I've never disassembled > > anything so wouldn't know where to check. > > Sorry for intervening. The values of these constants are known at compile time, > so this shouldn't create static a initializer. See > https://docs.google.com/a/google.com/document/d/1Zk9JZR3z1L7LMXbd05tj04rClYcu... > > [As a sanity check, you can try creating an enum with this value, and the code > just wouldn't compile if the operands values can't be determined at compile > time] That's a neat trick! Nice.
Address comments. Still don't look at instrumentation tests yet. https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/a... File android_webview/browser/aw_quota_manager_bridge.h (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/a... android_webview/browser/aw_quota_manager_bridge.h:10: // Empty super class so this can be owned by AwBrowserContext. On 2013/02/20 12:21:38, Martin Kosiba wrote: > On 2013/02/20 09:29:03, Mikhail Naganov (Chromium) wrote: > > I don't understand the usage of "super" here. Does this class act as a > > superclass for other classes? I didn't see any. > > probably "base class" is a better name. I'm assuming the idea is that you need > the public destructor for AwBrowserContext to be able to destroy the bridge. > Maybe add that to the comment so it's super-obvious :) Martin's completely right. Updated comment. https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/a... File android_webview/browser/aw_quota_permission_context.cc (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/a... android_webview/browser/aw_quota_permission_context.cc:26: // Android WebView only uses quota::kStorageTypeTemporary tyep of storage On 2013/02/20 09:29:03, Mikhail Naganov (Chromium) wrote: > nit: "tyep" Done. https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:179: QuotaManager* quota_manager = storage_partition->GetQuotaManager(); On 2013/02/20 12:21:38, Martin Kosiba wrote: > This bit me a couple of times so just double-checking with you. > Normally, if the object needs to be interacted with on the IO thread, you need > to get the instance on the IO thread too. I'm assuming that storage_partition is > UI-thread safe and the only thing it does with the QuotaManager is hold a > pointer to it (and create it? But then QuotaManager would be constructed on the > UI thread...) Good point. First, BrowsingDataRemover::RemoveImpl calls this on the UI thread. Also StoragePartitionImpl and its corresponding QuotaManager is created in StoragePartitionImpl::Create, which is run on the UI thread (apparently it's even allowed to be called before message loops are started). So, I'm safe by accident :) https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:187: int ALL_CLIENT_MASK = QuotaClient::kFileSystem | On 2013/02/20 19:49:33, joth wrote: > you should make it const. > > also, I think the kFoo style is preferred (modulo where we're sharing constant > across c++ & java) so I'd be tempted to use that here, given the individual > consts are kStyle. > > although, I see kAllClientsMask already declared in > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/quota/quota... > > why not using that? as you have it, you'd need to remember to update this too. > (is that this intent? if so, should probably comment it) In the documentation for QuotaManager::DeleteOriginData: "Setting the mask to QuotaClient::kAllClientsMask will remove all clients from the origin, regardless of type." We only want to remove temporary type, not all types. Added a comment. https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:189: QuotaClient::kAppcache | On 2013/02/20 09:29:03, Mikhail Naganov (Chromium) wrote: > On 2013/02/20 00:53:51, boliu wrote: > > Joth: Know if this will create a static initializer? I've never disassembled > > anything so wouldn't know where to check. > > Sorry for intervening. The values of these constants are known at compile time, > so this shouldn't create static a initializer. See > https://docs.google.com/a/google.com/document/d/1Zk9JZR3z1L7LMXbd05tj04rClYcu... > > [As a sanity check, you can try creating an enum with this value, and the code > just wouldn't compile if the operands values can't be determined at compile > time] The enum test failed with: error: 'android_webview::(anonymous namespace)::ALL_CLIENT_MASK' cannot appear in a constant-expression I'll take that as there *is* a static initializer. So made this into an anonymous function instead. https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:194: void DeleteOrigins( On 2013/02/20 12:21:38, Martin Kosiba wrote: > nit2: it would also be nice if the name indicated it's "dedicated" for being > invoked from the AwQuotaManagerBridgeImpl::DeleteAllData method. > > DeleteOriginDataWorker or DeleteAllOriginDataWorker? Don't like "Worker" moniker for a function. Changed to DeleteAllOriginData. https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:212: void AwQuotaManagerBridgeImpl::DeleteAllData(JNIEnv* env, jobject object) { On 2013/02/20 12:21:38, Martin Kosiba wrote: > nit: still think this should match the quota manager API more closely > (DeleteOriginData or DeleteAllOriginData). So these names match the public java WebStorage api. Not all of those methods map directly to QuotaManager, and this is an example, so I'm inclined to stick with the java name all these methods. https://codereview.chromium.org/12253057/diff/17001/base/android/jni_array.cc File base/android/jni_array.cc (right): https://codereview.chromium.org/12253057/diff/17001/base/android/jni_array.cc... base/android/jni_array.cc:29: JNIEnv* env, const int64* longs, size_t len) { On 2013/02/20 12:21:38, Martin Kosiba wrote: > I know the function above uses a raw C-style array but std::vector is just as > efficient, guaranteed to take up a continuous block of memory (so memcpy-ing it > is OK) and you don't risk mis-specifying the len. > > Also, it seems all ToJavaLongArray callsites use a vector. You probably should > add a wrapper that takes in a vector and calls this method to do the actual > work. Overloaded the method with a vector version and updated the test to reflect that. https://codereview.chromium.org/12253057/diff/17001/base/android/jni_array.cc... base/android/jni_array.cc:35: memcpy(elements, longs, len * sizeof(*longs)); On 2013/02/20 12:21:38, Martin Kosiba wrote: > I can't remember what the exact issue was but one of the C++ books I read > recommended using the type (int64) rather than the variable in this case. > I remember in one of the effective c++ books, the recommendation is the exact opposite, because then the sizeof and the type of the variable will not get out of sync. > I'd say that it's OK to hardcode the type since the method does explicitly deal > with longs (aka int64s) so the chances that you change the type of *longs and > forget to update the code are low. I don't really care much either way and I see sizeof with both styles under base/. So...I'll do whatever joth says? If he doesn't care, I'll use the type. https://codereview.chromium.org/12253057/diff/35001/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/35001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:226: make_scoped_refptr(quota_manager)))); With out make_scoped_refptr here, it would fail to compile with error: ../../base/bind.h:112:3: error: size of array is negative I guess whatever template magic that makes object argument on a method work (4 lines above) doesn't work if it's a simple function.
On 20 February 2013 12:14, <boliu@chromium.org> wrote: > Address comments. Still don't look at instrumentation tests yet. > > > > https://codereview.chromium.**org/12253057/diff/17001/** > android_webview/browser/aw_**quota_manager_bridge.h<https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/aw_quota_manager_bridge.h> > File android_webview/browser/aw_**quota_manager_bridge.h (right): > > https://codereview.chromium.**org/12253057/diff/17001/** > android_webview/browser/aw_**quota_manager_bridge.h#**newcode10<https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/aw_quota_manager_bridge.h#newcode10> > android_webview/browser/aw_**quota_manager_bridge.h:10: // Empty super > class so this can be owned by AwBrowserContext. > On 2013/02/20 12:21:38, Martin Kosiba wrote: > >> On 2013/02/20 09:29:03, Mikhail Naganov (Chromium) wrote: >> > I don't understand the usage of "super" here. Does this class act as >> > a > >> > superclass for other classes? I didn't see any. >> > > probably "base class" is a better name. I'm assuming the idea is that >> > you need > >> the public destructor for AwBrowserContext to be able to destroy the >> > bridge. > >> Maybe add that to the comment so it's super-obvious :) >> > > Martin's completely right. Updated comment. > > > https://codereview.chromium.**org/12253057/diff/17001/** > android_webview/browser/aw_**quota_permission_context.cc<https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/aw_quota_permission_context.cc> > File android_webview/browser/aw_**quota_permission_context.cc (right): > > https://codereview.chromium.**org/12253057/diff/17001/** > android_webview/browser/aw_**quota_permission_context.cc#**newcode26<https://codereview.chromium.org/12253057/diff/17001/android_webview/browser/aw_quota_permission_context.cc#newcode26> > android_webview/browser/aw_**quota_permission_context.cc:**26: // Android > WebView only uses quota::kStorageTypeTemporary tyep of storage > On 2013/02/20 09:29:03, Mikhail Naganov (Chromium) wrote: > >> nit: "tyep" >> > > Done. > > > https://codereview.chromium.**org/12253057/diff/17001/** > android_webview/native/aw_**quota_manager_bridge_impl.cc<https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw_quota_manager_bridge_impl.cc> > File android_webview/native/aw_**quota_manager_bridge_impl.cc (right): > > https://codereview.chromium.**org/12253057/diff/17001/** > android_webview/native/aw_**quota_manager_bridge_impl.cc#**newcode179<https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw_quota_manager_bridge_impl.cc#newcode179> > android_webview/native/aw_**quota_manager_bridge_impl.cc:**179: > QuotaManager* quota_manager = storage_partition->**GetQuotaManager(); > On 2013/02/20 12:21:38, Martin Kosiba wrote: > >> This bit me a couple of times so just double-checking with you. >> Normally, if the object needs to be interacted with on the IO thread, >> > you need > >> to get the instance on the IO thread too. I'm assuming that >> > storage_partition is > >> UI-thread safe and the only thing it does with the QuotaManager is >> > hold a > >> pointer to it (and create it? But then QuotaManager would be >> > constructed on the > >> UI thread...) >> > > Good point. > > First, BrowsingDataRemover::**RemoveImpl calls this on the UI thread. Also > StoragePartitionImpl and its corresponding QuotaManager is created in > StoragePartitionImpl::Create, which is run on the UI thread (apparently > it's even allowed to be called before message loops are started). > > So, I'm safe by accident :) > > > https://codereview.chromium.**org/12253057/diff/17001/** > android_webview/native/aw_**quota_manager_bridge_impl.cc#**newcode187<https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw_quota_manager_bridge_impl.cc#newcode187> > android_webview/native/aw_**quota_manager_bridge_impl.cc:**187: int > ALL_CLIENT_MASK = QuotaClient::kFileSystem | > On 2013/02/20 19:49:33, joth wrote: > >> you should make it const. >> > > also, I think the kFoo style is preferred (modulo where we're sharing >> > constant > >> across c++ & java) so I'd be tempted to use that here, given the >> > individual > >> consts are kStyle. >> > > although, I see kAllClientsMask already declared in >> > > https://code.google.com/p/**chromium/codesearch#chromium/** > src/webkit/quota/quota_client.**h&sq=package:chromium&type=cs&** > l=38&rcl=1361345605<https://code.google.com/p/chromium/codesearch#chromium/src/webkit/quota/quota_client.h&sq=package:chromium&type=cs&l=38&rcl=1361345605> > > why not using that? as you have it, you'd need to remember to update >> > this too. > >> (is that this intent? if so, should probably comment it) >> > > In the documentation for QuotaManager::**DeleteOriginData: "Setting the > mask to QuotaClient::kAllClientsMask will remove all clients from the > origin, regardless of type." We only want to remove temporary type, not > all types. > > Added a comment. > > > https://codereview.chromium.**org/12253057/diff/17001/** > android_webview/native/aw_**quota_manager_bridge_impl.cc#**newcode189<https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw_quota_manager_bridge_impl.cc#newcode189> > android_webview/native/aw_**quota_manager_bridge_impl.cc:**189: > QuotaClient::kAppcache | > On 2013/02/20 09:29:03, Mikhail Naganov (Chromium) wrote: > >> On 2013/02/20 00:53:51, boliu wrote: >> > Joth: Know if this will create a static initializer? I've never >> > disassembled > >> > anything so wouldn't know where to check. >> > > Sorry for intervening. The values of these constants are known at >> > compile time, > >> so this shouldn't create static a initializer. See >> > > https://docs.google.com/a/**google.com/document/d/** > 1Zk9JZR3z1L7LMXbd05tj04rClYcuJ**fQxECBU79eT5ZY/edit<https://docs.google.com/a/google.com/document/d/1Zk9JZR3z1L7LMXbd05tj04rClYcuJfQxECBU79eT5ZY/edit> > > [As a sanity check, you can try creating an enum with this value, and >> > the code > >> just wouldn't compile if the operands values can't be determined at >> > compile > >> time] >> > > The enum test failed with: > error: 'android_webview::(anonymous namespace)::ALL_CLIENT_MASK' cannot > appear in a constant-expression > > I'll take that as there *is* a static initializer. So made this into an > anonymous function instead. > > > https://codereview.chromium.**org/12253057/diff/17001/** > android_webview/native/aw_**quota_manager_bridge_impl.cc#**newcode194<https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw_quota_manager_bridge_impl.cc#newcode194> > android_webview/native/aw_**quota_manager_bridge_impl.cc:**194: void > DeleteOrigins( > On 2013/02/20 12:21:38, Martin Kosiba wrote: > >> nit2: it would also be nice if the name indicated it's "dedicated" for >> > being > >> invoked from the AwQuotaManagerBridgeImpl::**DeleteAllData method. >> > > DeleteOriginDataWorker or DeleteAllOriginDataWorker? >> > > Don't like "Worker" moniker for a function. Changed to > DeleteAllOriginData. > > > https://codereview.chromium.**org/12253057/diff/17001/** > android_webview/native/aw_**quota_manager_bridge_impl.cc#**newcode212<https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw_quota_manager_bridge_impl.cc#newcode212> > android_webview/native/aw_**quota_manager_bridge_impl.cc:**212: void > AwQuotaManagerBridgeImpl::**DeleteAllData(JNIEnv* env, jobject object) { > On 2013/02/20 12:21:38, Martin Kosiba wrote: > >> nit: still think this should match the quota manager API more closely >> (DeleteOriginData or DeleteAllOriginData). >> > > So these names match the public java WebStorage api. Not all of those > methods map directly to QuotaManager, and this is an example, so I'm > inclined to stick with the java name all these methods. > > > https://codereview.chromium.**org/12253057/diff/17001/base/** > android/jni_array.cc<https://codereview.chromium.org/12253057/diff/17001/base/android/jni_array.cc> > File base/android/jni_array.cc (right): > > https://codereview.chromium.**org/12253057/diff/17001/base/** > android/jni_array.cc#newcode29<https://codereview.chromium.org/12253057/diff/17001/base/android/jni_array.cc#newcode29> > base/android/jni_array.cc:29: JNIEnv* env, const int64* longs, size_t > len) { > On 2013/02/20 12:21:38, Martin Kosiba wrote: > >> I know the function above uses a raw C-style array but std::vector is >> > just as > >> efficient, guaranteed to take up a continuous block of memory (so >> > memcpy-ing it > >> is OK) and you don't risk mis-specifying the len. >> > > Also, it seems all ToJavaLongArray callsites use a vector. You >> > probably should > >> add a wrapper that takes in a vector and calls this method to do the >> > actual > >> work. >> > > Overloaded the method with a vector version and updated the test to > reflect that. > > > https://codereview.chromium.**org/12253057/diff/17001/base/** > android/jni_array.cc#newcode35<https://codereview.chromium.org/12253057/diff/17001/base/android/jni_array.cc#newcode35> > base/android/jni_array.cc:35: memcpy(elements, longs, len * > sizeof(*longs)); > On 2013/02/20 12:21:38, Martin Kosiba wrote: > >> I can't remember what the exact issue was but one of the C++ books I >> > read > >> recommended using the type (int64) rather than the variable in this >> > case. > > > I remember in one of the effective c++ books, the recommendation is the > exact opposite, because then the sizeof and the type of the variable > will not get out of sync. > > > I'd say that it's OK to hardcode the type since the method does >> > explicitly deal > >> with longs (aka int64s) so the chances that you change the type of >> > *longs and > >> forget to update the code are low. >> > > I don't really care much either way and I see sizeof with both styles > under base/. So...I'll do whatever joth says? If he doesn't care, I'll > use the type. > > https://codereview.chromium.**org/12253057/diff/35001/** > android_webview/native/aw_**quota_manager_bridge_impl.cc<https://codereview.chromium.org/12253057/diff/35001/android_webview/native/aw_quota_manager_bridge_impl.cc> > File android_webview/native/aw_**quota_manager_bridge_impl.cc (right): > > https://codereview.chromium.**org/12253057/diff/35001/** > android_webview/native/aw_**quota_manager_bridge_impl.cc#**newcode226<https://codereview.chromium.org/12253057/diff/35001/android_webview/native/aw_quota_manager_bridge_impl.cc#newcode226> > android_webview/native/aw_**quota_manager_bridge_impl.cc:**226: > make_scoped_refptr(quota_**manager)))); > With out make_scoped_refptr here, it would fail to compile with error: > ../../base/bind.h:112:3: error: size of array is negative > > I guess whatever template magic that makes object argument on a method > work (4 lines above) doesn't work if it's a simple function. > > "error: size of array is negative" is code for "you hit a compile assert" bind.h:112 (ish) it gives the 'real' error message " p1_is_refcounted_type_and_needs_scoped_refptr" :- // For methods, we need to be careful for parameter 1. We do not require // a scoped_refptr because BindState<> itself takes care of AddRef() for // methods. We also disallow binding of an array as the method's target // object. COMPILE_ASSERT( internal::HasIsMethodTag<RunnableType>::value || !internal::NeedsScopedRefptrButGetsRawPtr<P1>::value, p1_is_refcounted_type_and_needs_scoped_refptr); ...it's an error to pass a ref-counted object as first param to a simple function without passing it as a scoped_refptr? you could avoid the make_scoped_refptr by declaring |quota_manager| to be of type scoped_refptr<> to start with?
https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:187: int ALL_CLIENT_MASK = QuotaClient::kFileSystem | On 2013/02/20 20:14:16, boliu wrote: > On 2013/02/20 19:49:33, joth wrote: > > you should make it const. > > > > also, I think the kFoo style is preferred (modulo where we're sharing constant > > across c++ & java) so I'd be tempted to use that here, given the individual > > consts are kStyle. > > > > although, I see kAllClientsMask already declared in > > > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/quota/quota... > > > > why not using that? as you have it, you'd need to remember to update this too. > > (is that this intent? if so, should probably comment it) > > In the documentation for QuotaManager::DeleteOriginData: "Setting the mask to > QuotaClient::kAllClientsMask will remove all clients from the origin, regardless > of type." We only want to remove temporary type, not all types. > > Added a comment. I think we are misinterpreting the comment, as looking at the actual code, there is nothing special being done to the type specified for the kAllClientsMask case. The type is always being passed down to quota manager clients. https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:189: QuotaClient::kAppcache | On 2013/02/20 20:14:16, boliu wrote: > On 2013/02/20 09:29:03, Mikhail Naganov (Chromium) wrote: > > On 2013/02/20 00:53:51, boliu wrote: > > > Joth: Know if this will create a static initializer? I've never disassembled > > > anything so wouldn't know where to check. > > > > Sorry for intervening. The values of these constants are known at compile > time, > > so this shouldn't create static a initializer. See > > > https://docs.google.com/a/google.com/document/d/1Zk9JZR3z1L7LMXbd05tj04rClYcu... > > > > [As a sanity check, you can try creating an enum with this value, and the code > > just wouldn't compile if the operands values can't be determined at compile > > time] > > The enum test failed with: > error: 'android_webview::(anonymous namespace)::ALL_CLIENT_MASK' cannot appear > in a constant-expression > > I'll take that as there *is* a static initializer. So made this into an > anonymous function instead. Sorry, are you trying to write "enum { X = ALL_CLIENT_MASK }"? This will not work unless you declare ALL_CLIENT_MASK as a "const int" (and maybe even in that case, I'm not sure). What I meant was trying to write "enum { ALL_CLIENT_MASK = QuotaClient::kFileSystem | ... }"
I'll upload a new patch when the tests are done https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:187: int ALL_CLIENT_MASK = QuotaClient::kFileSystem | On 2013/02/20 21:54:34, Mikhail Naganov (Chromium) wrote: > On 2013/02/20 20:14:16, boliu wrote: > > On 2013/02/20 19:49:33, joth wrote: > > > you should make it const. > > > > > > also, I think the kFoo style is preferred (modulo where we're sharing > constant > > > across c++ & java) so I'd be tempted to use that here, given the individual > > > consts are kStyle. > > > > > > although, I see kAllClientsMask already declared in > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/quota/quota... > > > > > > why not using that? as you have it, you'd need to remember to update this > too. > > > (is that this intent? if so, should probably comment it) > > > > In the documentation for QuotaManager::DeleteOriginData: "Setting the mask to > > QuotaClient::kAllClientsMask will remove all clients from the origin, > regardless > > of type." We only want to remove temporary type, not all types. > > > > Added a comment. > > I think we are misinterpreting the comment, as looking at the actual code, there > is nothing special being done to the type specified for the kAllClientsMask > case. The type is always being passed down to quota manager clients. You are right. It's a confusing comment. And I learned that enums are basically unsigned so two's compliment makes -1 equivalent to 0xffff... the all inclusive mask, no cast required! https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:189: QuotaClient::kAppcache | On 2013/02/20 21:54:34, Mikhail Naganov (Chromium) wrote: > On 2013/02/20 20:14:16, boliu wrote: > > On 2013/02/20 09:29:03, Mikhail Naganov (Chromium) wrote: > > > On 2013/02/20 00:53:51, boliu wrote: > > > > Joth: Know if this will create a static initializer? I've never > disassembled > > > > anything so wouldn't know where to check. > > > > > > Sorry for intervening. The values of these constants are known at compile > > time, > > > so this shouldn't create static a initializer. See > > > > > > https://docs.google.com/a/google.com/document/d/1Zk9JZR3z1L7LMXbd05tj04rClYcu... > > > > > > [As a sanity check, you can try creating an enum with this value, and the > code > > > just wouldn't compile if the operands values can't be determined at compile > > > time] > > > > The enum test failed with: > > error: 'android_webview::(anonymous namespace)::ALL_CLIENT_MASK' cannot appear > > in a constant-expression > > > > I'll take that as there *is* a static initializer. So made this into an > > anonymous function instead. > > Sorry, are you trying to write "enum { X = ALL_CLIENT_MASK }"? This will not > work unless you declare ALL_CLIENT_MASK as a "const int" (and maybe even in that > case, I'm not sure). > > What I meant was trying to write "enum { ALL_CLIENT_MASK = > QuotaClient::kFileSystem | ... }" And you are right again. I did it the first way. The second way does indeed compile. But now I don't need it anymore :)
Tests still work in progress, but getting there. https://codereview.chromium.org/12253057/diff/31004/android_webview/javatests... File android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java (right): https://codereview.chromium.org/12253057/diff/31004/android_webview/javatests... android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java:195: assertTrue(getUsageForOrigin(mOrigin) > 0); This passes if we use getOrigins to check the usage. So something is still wrong here, need to check.
https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/17001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:212: void AwQuotaManagerBridgeImpl::DeleteAllData(JNIEnv* env, jobject object) { On 2013/02/20 20:14:16, boliu wrote: > On 2013/02/20 12:21:38, Martin Kosiba wrote: > > nit: still think this should match the quota manager API more closely > > (DeleteOriginData or DeleteAllOriginData). > > So these names match the public java WebStorage api. Not all of those methods > map directly to QuotaManager, and this is an example, so I'm inclined to stick > with the java name all these methods. ah, OK. https://codereview.chromium.org/12253057/diff/35001/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/35001/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:226: make_scoped_refptr(quota_manager)))); On 2013/02/20 20:14:16, boliu wrote: > With out make_scoped_refptr here, it would fail to compile with error: > ../../base/bind.h:112:3: error: size of array is negative > > I guess whatever template magic that makes object argument on a method work (4 > lines above) doesn't work if it's a simple function. ah.. it seems like it's fine for class methods (so if "this" is ref-counted) but not ok for "random" method parameters. My bad for pointing you in the wrong direction. https://codereview.chromium.org/12253057/diff/31004/android_webview/javatests... File android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java (right): https://codereview.chromium.org/12253057/diff/31004/android_webview/javatests... android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java:53: private AwQuotaManagerBridge getQuotaManagerBridge() throws Throwable { nit: this should throw an Exception, not Throwable. https://codereview.chromium.org/12253057/diff/31004/android_webview/javatests... android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java:62: private void deleteAllData() throws Throwable { nit: same here and in any method that isn't a test. https://codereview.chromium.org/12253057/diff/31004/android_webview/javatests... android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java:83: AwQuotaManagerBridge.Origins mOrigins; nit: please expose a getOrigins() method that asserts getCallCount() > 0 and make the field private. https://codereview.chromium.org/12253057/diff/31004/android_webview/javatests... android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java:115: long mValue; same thing here, please use a getter method. https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:128: base::Bind(&GetOriginsTask::DoneOnUIThread, this)); umm... you should be able to do something like this: BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, base::Bind(ui_callback_, origin_, usage_, quota_)); That should work as the base::Bind should produce a Closure (which is a Callback with 0 args) in this and that's exactly what PostTask takes as the 3rd argument. Go ahead and read the docs in base/callback.h (PARTIAL BINDING OF PARAMETERS section) if you don't believe me :P https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:264: base::android::ToJavaLongArray(env, usage.begin(), usage.size()).obj(), use the version that takes the vector as an arg directly. https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:292: base::Bind(&RunGetUsageAndQuotaCallback, same thing here - you should be able to bind the callback directly.
Just replying. No new patch. https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:128: base::Bind(&GetOriginsTask::DoneOnUIThread, this)); On 2013/02/21 11:38:30, Martin Kosiba wrote: > umm... you should be able to do something like this: > > BrowserThread::PostTask( > BrowserThread::UI, > FROM_HERE, > base::Bind(ui_callback_, origin_, usage_, quota_)); > > That should work as the base::Bind should produce a Closure (which is a Callback > with 0 args) in this and that's exactly what PostTask takes as the 3rd argument. > Go ahead and read the docs in base/callback.h (PARTIAL BINDING OF PARAMETERS > section) if you don't believe me :P Oh wow it does work. I thought media::base::BindToLoop was my only salvation... But this raises another problem: I assume the 3 (possibly big-ish) vectors are copied into the callback. I'd avoid the copy here so keeping this one as is. (Although it does highlight the fact that this object lives on 2 threads...it's a trade off I guess). https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:264: base::android::ToJavaLongArray(env, usage.begin(), usage.size()).obj(), On 2013/02/21 11:38:30, Martin Kosiba wrote: > use the version that takes the vector as an arg directly. Wah?? I was sure I fixed these...must have gotten lost in random git checkouts... https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:292: base::Bind(&RunGetUsageAndQuotaCallback, On 2013/02/21 11:38:30, Martin Kosiba wrote: > same thing here - you should be able to bind the callback directly. Fixed here. Bind ftw!!
Finally got some real tests passing. Still needs comments all around. But otherwise everything ready for review. https://codereview.chromium.org/12253057/diff/31004/android_webview/javatests... File android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java (right): https://codereview.chromium.org/12253057/diff/31004/android_webview/javatests... android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java:53: private AwQuotaManagerBridge getQuotaManagerBridge() throws Throwable { On 2013/02/21 11:38:30, Martin Kosiba wrote: > nit: this should throw an Exception, not Throwable. Apparently the tests can just throw Exception as well. Not a big fan of java checked exceptions right about now... https://codereview.chromium.org/12253057/diff/31004/android_webview/javatests... android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java:83: AwQuotaManagerBridge.Origins mOrigins; On 2013/02/21 11:38:30, Martin Kosiba wrote: > nit: please expose a getOrigins() method that asserts getCallCount() > 0 and > make the field private. Done. https://codereview.chromium.org/12253057/diff/31004/android_webview/javatests... android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java:115: long mValue; On 2013/02/21 11:38:30, Martin Kosiba wrote: > same thing here, please use a getter method. Done. https://codereview.chromium.org/12253057/diff/31004/android_webview/javatests... android_webview/javatests/src/org/chromium/android_webview/test/AwQuotaManagerBridgeTest.java:118: mValue = mValue; And the problem was this. Fixed https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:263: base::android::ToJavaArrayOfStrings(env, origin).obj(), Oh wow this is so so wrong...it's a byte[][] on java side https://codereview.chromium.org/12253057/diff/27002/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/27002/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:219: // TODO(boliu): This needs to clear WebStorage (ie localStorage and Will do this in a follow up.
Ready for full review.
fly by commentage. https://codereview.chromium.org/12253057/diff/28003/android_webview/browser/n... File android_webview/browser/native_factory.h (right): https://codereview.chromium.org/12253057/diff/28003/android_webview/browser/n... android_webview/browser/native_factory.h:20: class NativeFactory { native is a bit vague here... maybe JavaBridgeFactory or JniDependencyFactory or some variation? https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java (right): https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:39: * are optimized for JNI convenience and need to be converted. I was going to note that WebStorage.Origin is a "primitive" type, so it is acceptable to use it directly here, but I see it's conveniently only got hidden constructors... https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:41: public static class Origins { maybe comment these are parallel arrays, so should always be equal in length. https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:61: private Map<Integer, ValueCallback<Long>> mPendingGetUsageForOriginCallbacks; An alternative approach here is to pass a jweak to the ValueCallback<> down to native for it to callthrough when it completes, which maybe simpler? Guess you would also need a (single) Set<Object> here to hold a java-side strong pointer to the callback to keep it in scope until the callback arrives. https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:89: * TODO(boliu): Actually clear 1. 1 ? https://codereview.chromium.org/12253057/diff/28003/android_webview/lib/main/... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/12253057/diff/28003/android_webview/lib/main/... android_webview/lib/main/aw_main_delegate.cc:90: return AwGeolocationPermissionContext::Create(); probably should pass through the browser context here? https://codereview.chromium.org/12253057/diff/28003/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/28003/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:307: nit: excess \n
https://codereview.chromium.org/12253057/diff/28003/android_webview/browser/n... File android_webview/browser/native_factory.h (right): https://codereview.chromium.org/12253057/diff/28003/android_webview/browser/n... android_webview/browser/native_factory.h:20: class NativeFactory { On 2013/02/21 22:57:52, joth wrote: > native is a bit vague here... maybe JavaBridgeFactory or JniDependencyFactory or > some variation? Renamed to JniDependencyFactory https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java (right): https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:39: * are optimized for JNI convenience and need to be converted. On 2013/02/21 22:57:52, joth wrote: > I was going to note that WebStorage.Origin is a "primitive" type, so it is > acceptable to use it directly here, but I see it's conveniently only got hidden > constructors... Yep... https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:41: public static class Origins { On 2013/02/21 22:57:52, joth wrote: > maybe comment these are parallel arrays, so should always be equal in length. Done. https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:61: private Map<Integer, ValueCallback<Long>> mPendingGetUsageForOriginCallbacks; On 2013/02/21 22:57:52, joth wrote: > An alternative approach here is to pass a jweak to the ValueCallback<> down to > native for it to callthrough when it completes, which maybe simpler? > Guess you would also need a (single) Set<Object> here to hold a java-side strong > pointer to the callback to keep it in scope until the callback arrives. So Martin is already working on generalizing something like this here: https://codereview.chromium.org/12313042/ With your solution, what would remove the callback from the Set<Object> after it's called? https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:89: * TODO(boliu): Actually clear 1. On 2013/02/21 22:57:52, joth wrote: > 1 ? Web Storage. https://codereview.chromium.org/12253057/diff/28003/android_webview/lib/main/... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/12253057/diff/28003/android_webview/lib/main/... android_webview/lib/main/aw_main_delegate.cc:90: return AwGeolocationPermissionContext::Create(); On 2013/02/21 22:57:52, joth wrote: > probably should pass through the browser context here? Passed, but just ignored in AwGeolocationPermissionContext::Create right now. Maybe it'll be useful one day... https://codereview.chromium.org/12253057/diff/28003/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/28003/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:307: On 2013/02/21 22:57:52, joth wrote: > nit: excess \n Done.
On 21 February 2013 16:07, <boliu@chromium.org> wrote: > > https://codereview.chromium.**org/12253057/diff/28003/** > android_webview/browser/**native_factory.h<https://codereview.chromium.org/12253057/diff/28003/android_webview/browser/native_factory.h> > File android_webview/browser/**native_factory.h (right): > > https://codereview.chromium.**org/12253057/diff/28003/** > android_webview/browser/**native_factory.h#newcode20<https://codereview.chromium.org/12253057/diff/28003/android_webview/browser/native_factory.h#newcode20> > android_webview/browser/**native_factory.h:20: class NativeFactory { > On 2013/02/21 22:57:52, joth wrote: > >> native is a bit vague here... maybe JavaBridgeFactory or >> > JniDependencyFactory or > >> some variation? >> > > Renamed to JniDependencyFactory > > > https://codereview.chromium.**org/12253057/diff/28003/** > android_webview/java/src/org/**chromium/android_webview/** > AwQuotaManagerBridge.java<https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java> > File > android_webview/java/src/org/**chromium/android_webview/** > AwQuotaManagerBridge.java > (right): > > https://codereview.chromium.**org/12253057/diff/28003/** > android_webview/java/src/org/**chromium/android_webview/** > AwQuotaManagerBridge.java#**newcode39<https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java#newcode39> > android_webview/java/src/org/**chromium/android_webview/** > AwQuotaManagerBridge.java:39: > * are optimized for JNI convenience and need to be converted. > On 2013/02/21 22:57:52, joth wrote: > >> I was going to note that WebStorage.Origin is a "primitive" type, so >> > it is > >> acceptable to use it directly here, but I see it's conveniently only >> > got hidden > >> constructors... >> > > Yep... > > > https://codereview.chromium.**org/12253057/diff/28003/** > android_webview/java/src/org/**chromium/android_webview/** > AwQuotaManagerBridge.java#**newcode41<https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java#newcode41> > android_webview/java/src/org/**chromium/android_webview/** > AwQuotaManagerBridge.java:41: > public static class Origins { > On 2013/02/21 22:57:52, joth wrote: > >> maybe comment these are parallel arrays, so should always be equal in >> > length. > > Done. > > > https://codereview.chromium.**org/12253057/diff/28003/** > android_webview/java/src/org/**chromium/android_webview/** > AwQuotaManagerBridge.java#**newcode61<https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java#newcode61> > android_webview/java/src/org/**chromium/android_webview/** > AwQuotaManagerBridge.java:61: > private Map<Integer, ValueCallback<Long>> > mPendingGetUsageForOriginCallb**acks; > On 2013/02/21 22:57:52, joth wrote: > >> An alternative approach here is to pass a jweak to the ValueCallback<> >> > down to > >> native for it to callthrough when it completes, which maybe simpler? >> Guess you would also need a (single) Set<Object> here to hold a >> > java-side strong > >> pointer to the callback to keep it in scope until the callback >> > arrives. > > So Martin is already working on generalizing something like this here: > https://codereview.chromium.**org/12313042/<https://codereview.chromium.org/1... > > With your solution, what would remove the callback from the Set<Object> > after it's called? > > you'd probably still have a helper method to fire the ValueCallback java side, and that can remove from the set too yeah lets see how martin's approach pans out though. A couple utility methods could help avoid us having to keep re-inventing almost the same pattern over and again > > https://codereview.chromium.**org/12253057/diff/28003/** > android_webview/java/src/org/**chromium/android_webview/** > AwQuotaManagerBridge.java#**newcode89<https://codereview.chromium.org/12253057/diff/28003/android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java#newcode89> > android_webview/java/src/org/**chromium/android_webview/** > AwQuotaManagerBridge.java:89: > * TODO(boliu): Actually clear 1. > On 2013/02/21 22:57:52, joth wrote: > >> 1 ? >> > > Web Storage. > > > https://codereview.chromium.**org/12253057/diff/28003/** > android_webview/lib/main/aw_**main_delegate.cc<https://codereview.chromium.org/12253057/diff/28003/android_webview/lib/main/aw_main_delegate.cc> > File android_webview/lib/main/aw_**main_delegate.cc (right): > > https://codereview.chromium.**org/12253057/diff/28003/** > android_webview/lib/main/aw_**main_delegate.cc#newcode90<https://codereview.chromium.org/12253057/diff/28003/android_webview/lib/main/aw_main_delegate.cc#newcode90> > android_webview/lib/main/aw_**main_delegate.cc:90: return > AwGeolocationPermissionContext**::Create(); > On 2013/02/21 22:57:52, joth wrote: > >> probably should pass through the browser context here? >> > > Passed, but just ignored in AwGeolocationPermissionContext**::Create right > now. Maybe it'll be useful one day... > > > https://codereview.chromium.**org/12253057/diff/28003/** > android_webview/native/aw_**quota_manager_bridge_impl.cc<https://codereview.chromium.org/12253057/diff/28003/android_webview/native/aw_quota_manager_bridge_impl.cc> > File android_webview/native/aw_**quota_manager_bridge_impl.cc (right): > > https://codereview.chromium.**org/12253057/diff/28003/** > android_webview/native/aw_**quota_manager_bridge_impl.cc#**newcode307<https://codereview.chromium.org/12253057/diff/28003/android_webview/native/aw_quota_manager_bridge_impl.cc#newcode307> > android_webview/native/aw_**quota_manager_bridge_impl.cc:**307: > On 2013/02/21 22:57:52, joth wrote: > >> nit: excess \n >> > > Done. > > https://codereview.chromium.**org/12253057/<https://codereview.chromium.org/1... >
a few nits. I will look more tomorrow. https://codereview.chromium.org/12253057/diff/23008/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java (right): https://codereview.chromium.org/12253057/diff/23008/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:42: // Origin, usage, and quota data in parallel arrays of same length. maybe we should be consistent in plural and singular names? these are all array? https://codereview.chromium.org/12253057/diff/23008/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:115: * Implements WebStorage.getOrigins. Get the quota of APIs 2-5 in aggregate for given origin. copy paste error? I think this should be WebStorage.getQuota. https://codereview.chromium.org/12253057/diff/23008/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:125: * Implements WebStorage.getOrigins. Get the usage of APIs 2-5 in aggregate for given origin. WebStorage.getUsage?
https://codereview.chromium.org/12253057/diff/23008/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java (right): https://codereview.chromium.org/12253057/diff/23008/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:42: // Origin, usage, and quota data in parallel arrays of same length. On 2013/02/22 02:44:02, sgurun wrote: > maybe we should be consistent in plural and singular names? these are all array? Wikitionary confirms: usage -> usages quota -> quotas Done https://codereview.chromium.org/12253057/diff/23008/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:115: * Implements WebStorage.getOrigins. Get the quota of APIs 2-5 in aggregate for given origin. On 2013/02/22 02:44:02, sgurun wrote: > copy paste error? I think this should be WebStorage.getQuota. Done. https://codereview.chromium.org/12253057/diff/23008/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:125: * Implements WebStorage.getOrigins. Get the usage of APIs 2-5 in aggregate for given origin. On 2013/02/22 02:44:02, sgurun wrote: > WebStorage.getUsage? Done.
lgtm https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:128: base::Bind(&GetOriginsTask::DoneOnUIThread, this)); On 2013/02/21 17:01:57, boliu wrote: > On 2013/02/21 11:38:30, Martin Kosiba wrote: > > umm... you should be able to do something like this: > > > > BrowserThread::PostTask( > > BrowserThread::UI, > > FROM_HERE, > > base::Bind(ui_callback_, origin_, usage_, quota_)); > > > > That should work as the base::Bind should produce a Closure (which is a > Callback > > with 0 args) in this and that's exactly what PostTask takes as the 3rd > argument. > > Go ahead and read the docs in base/callback.h (PARTIAL BINDING OF PARAMETERS > > section) if you don't believe me :P > > Oh wow it does work. I thought media::base::BindToLoop was my only salvation... > > But this raises another problem: I assume the 3 (possibly big-ish) vectors are > copied into the callback. I'd avoid the copy here so keeping this one as is. > (Although it does highlight the fact that this object lives on 2 threads...it's > a trade off I guess). _if_ you want to be super-an..correct about that you could do something like: vector<GURL>* origin = new vector<GURL>(); origin.swap(origin_); // same for usage and quota BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, base::Bind(ui_callback_, base::Owned(origin), base::Owned(usage), base::Owned(quota))); That way the data is explicitly passed between threads and there shouldn't be an extra copy. You'd need to change the callback signature to use pointers though.. Anyway.. I wouldn't worry about this too much.
lgtm % comments https://codereview.chromium.org/12253057/diff/36006/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java (right): https://codereview.chromium.org/12253057/diff/36006/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:30: assert Looper.myLooper() == Looper.getMainLooper(); nit: Can you use org.chromium.base.ThreadUtils.runningOnUiThread() ? https://codereview.chromium.org/12253057/diff/36006/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:79: * There are five HTML5 offline storage APIs. Not all of them are enabled for Android WebView. nit: Perhaps, specify which are enabled? https://codereview.chromium.org/12253057/diff/36006/android_webview/lib/aw_br... File android_webview/lib/aw_browser_dependency_factory_impl.cc (right): https://codereview.chromium.org/12253057/diff/36006/android_webview/lib/aw_br... android_webview/lib/aw_browser_dependency_factory_impl.cc:7: #include "android_webview/browser/aw_browser_context.h" Is this needed?
https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... File android_webview/native/aw_quota_manager_bridge_impl.cc (right): https://codereview.chromium.org/12253057/diff/31004/android_webview/native/aw... android_webview/native/aw_quota_manager_bridge_impl.cc:128: base::Bind(&GetOriginsTask::DoneOnUIThread, this)); On 2013/02/22 15:47:08, Martin Kosiba wrote: > On 2013/02/21 17:01:57, boliu wrote: > > On 2013/02/21 11:38:30, Martin Kosiba wrote: > > > umm... you should be able to do something like this: > > > > > > BrowserThread::PostTask( > > > BrowserThread::UI, > > > FROM_HERE, > > > base::Bind(ui_callback_, origin_, usage_, quota_)); > > > > > > That should work as the base::Bind should produce a Closure (which is a > > Callback > > > with 0 args) in this and that's exactly what PostTask takes as the 3rd > > argument. > > > Go ahead and read the docs in base/callback.h (PARTIAL BINDING OF PARAMETERS > > > section) if you don't believe me :P > > > > Oh wow it does work. I thought media::base::BindToLoop was my only > salvation... > > > > But this raises another problem: I assume the 3 (possibly big-ish) vectors are > > copied into the callback. I'd avoid the copy here so keeping this one as is. > > (Although it does highlight the fact that this object lives on 2 > threads...it's > > a trade off I guess). > > _if_ you want to be super-an..correct about that you could do something like: > > vector<GURL>* origin = new vector<GURL>(); > origin.swap(origin_); > // same for usage and quota > > BrowserThread::PostTask( > BrowserThread::UI, > FROM_HERE, > base::Bind(ui_callback_, base::Owned(origin), base::Owned(usage), > base::Owned(quota))); > > That way the data is explicitly passed between threads and there shouldn't be an > extra copy. You'd need to change the callback signature to use pointers though.. > Anyway.. I wouldn't worry about this too much. Ha! That involves new-ing 3 empty vectors, and the code is not as readable as this. I guess there is such a thing is going too far with callbacks :) https://codereview.chromium.org/12253057/diff/36006/android_webview/java/src/... File android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java (right): https://codereview.chromium.org/12253057/diff/36006/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:30: assert Looper.myLooper() == Looper.getMainLooper(); On 2013/02/22 16:09:15, Mikhail Naganov (Chromium) wrote: > nit: Can you use org.chromium.base.ThreadUtils.runningOnUiThread() ? There is even a handy ThreadUtils.assertOnUiThread. Updated here and in getNextId. https://codereview.chromium.org/12253057/diff/36006/android_webview/java/src/... android_webview/java/src/org/chromium/android_webview/AwQuotaManagerBridge.java:79: * There are five HTML5 offline storage APIs. Not all of them are enabled for Android WebView. On 2013/02/22 16:09:15, Mikhail Naganov (Chromium) wrote: > nit: Perhaps, specify which are enabled? I don't actually know...whatever clank supports I guess. Taking this comment out. https://codereview.chromium.org/12253057/diff/36006/android_webview/lib/aw_br... File android_webview/lib/aw_browser_dependency_factory_impl.cc (right): https://codereview.chromium.org/12253057/diff/36006/android_webview/lib/aw_br... android_webview/lib/aw_browser_dependency_factory_impl.cc:7: #include "android_webview/browser/aw_browser_context.h" On 2013/02/22 16:09:15, Mikhail Naganov (Chromium) wrote: > Is this needed? Yes, because this include is removed in aw_content_browser_client.h.
lgtm for base/ (and rest too, but I didn't read every line)
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/boliu@chromium.org/12253057/30015
Message was sent while issue was closed.
Change committed as 184139 |
