Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1631603002:
Web restrictions component only. (Closed)
|
Created:
4 years, 11 months ago by knn Modified:
4 years, 10 months ago CC:
blundell+watchlist_chromium.org, chromium-reviews, droger+watchlist_chromium.org, sdefresne+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWeb restrictions component only.
Splitting away non webview specific pieces from https://codereview.chromium.org/1423713015
so that Anthony can work parallely on the chrome bits while the webview bits are reviewed.
The individual pieces are:
A content resolver based web restriction provider.
A gin wrapper and a resource throttle to tie it together.
BUG=569879
Patch Set 1 #
Total comments: 35
Patch Set 2 : rebase #
Total comments: 4
Patch Set 3 : #Patch Set 4 : comments #
Total comments: 16
Patch Set 5 : use sequenced task runner #
Total comments: 6
Patch Set 6 : moar comments #
Total comments: 13
Patch Set 7 : compare with ps5 #Patch Set 8 : rm thread_checker #
Total comments: 10
Patch Set 9 : edge case #Patch Set 10 : destruct anywhere #
Total comments: 11
Patch Set 11 : comments #Patch Set 12 : #
Total comments: 4
Messages
Total messages: 38 (4 generated)
knn@chromium.org changed reviewers: + aberent@chromium.org, bauerb@chromium.org
PTAL. Thanks!
https://codereview.chromium.org/1631603002/diff/1/components/web_restriction.... File components/web_restriction.gypi (right): https://codereview.chromium.org/1631603002/diff/1/components/web_restriction.... components/web_restriction.gypi:34: #GN: //components/web_restriction:web_restriction Nit: Space after the comment character (#) for readability. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... File components/web_restriction/content_resolver_web_restriction_provider.cc (right): https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.cc:57: } Empty line afterwards https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.cc:74: initialized_ = false; You can do this as part of the initializer list (`initialized_(false)`). https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... File components/web_restriction/content_resolver_web_restriction_provider.h (right): https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:28: class SelfDeletingCallback { Why is this in the header? We should be able to move it to the implementation file into an anonymous namespace. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:51: ~SelfDeletingCallback(); Methods come before members. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:65: // Verify the content provider and Setup. Does this method verify the setup (i.e. is "setup" a noun here)? If not, and it sets [something] up, split it into two words. Also, no need to capitalize. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:75: bool GetErrorHtml(const GURL& url, std::string* error_html) const override; Identifying the error page by its URL isn't particularly pretty (for example, this implicitly contains the assumption that the error page would only depend on the URL). Is there some other identifier that we have when creating the throttle that we could use instead? https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:84: // Setup during Initialize. Also, "Set up [...]". https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:89: std::map<GURL, bool> url_access_cache_; How are we dealing with cache invalidation here? Also, you could use base/containers/mru_cache.h for this. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... File components/web_restriction/web_restriction_provider.h (right): https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/web_restriction_provider.h:14: enum UrlAccess { Move this into the class?
PTAL. Thanks! https://codereview.chromium.org/1631603002/diff/1/components/web_restriction.... File components/web_restriction.gypi (right): https://codereview.chromium.org/1631603002/diff/1/components/web_restriction.... components/web_restriction.gypi:34: #GN: //components/web_restriction:web_restriction On 2016/01/26 11:09:02, Bernhard Bauer wrote: > Nit: Space after the comment character (#) for readability. Done. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... File components/web_restriction/content_resolver_web_restriction_provider.cc (right): https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.cc:57: } On 2016/01/26 11:09:02, Bernhard Bauer wrote: > Empty line afterwards Done. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.cc:74: initialized_ = false; On 2016/01/26 11:09:02, Bernhard Bauer wrote: > You can do this as part of the initializer list (`initialized_(false)`). Done. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... File components/web_restriction/content_resolver_web_restriction_provider.h (right): https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:28: class SelfDeletingCallback { On 2016/01/26 11:09:03, Bernhard Bauer wrote: > Why is this in the header? We should be able to move it to the implementation > file into an anonymous namespace. We need this class to be declared before the jni header include in the cc file. It is either this or have a separate h/cc file for the callback which in turn doesn't make sense as it is very tightly coupled with the provider class and we will have include ordering issues. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:51: ~SelfDeletingCallback(); On 2016/01/26 11:09:03, Bernhard Bauer wrote: > Methods come before members. But this is a destructor and seems appropriate to put it with the constructor below which in turn always shows up in the end. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:65: // Verify the content provider and Setup. On 2016/01/26 11:09:03, Bernhard Bauer wrote: > Does this method verify the setup (i.e. is "setup" a noun here)? If not, and it > sets [something] up, split it into two words. Also, no need to capitalize. Done. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:75: bool GetErrorHtml(const GURL& url, std::string* error_html) const override; On 2016/01/26 11:09:03, Bernhard Bauer wrote: > Identifying the error page by its URL isn't particularly pretty (for example, > this implicitly contains the assumption that the error page would only depend on > the URL). Is there some other identifier that we have when creating the throttle > that we could use instead? That is the only information available to the provider right now (other than intrinsic factors like time I guess). Do you foresee major issues with this? https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:84: // Setup during Initialize. On 2016/01/26 11:09:03, Bernhard Bauer wrote: > Also, "Set up [...]". Done. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:89: std::map<GURL, bool> url_access_cache_; On 2016/01/26 11:09:03, Bernhard Bauer wrote: > How are we dealing with cache invalidation here? Also, you could use > base/containers/mru_cache.h for this. We aren't. I was banking on a webview app's lifetime being limited usually. Reason for not using MRUCache was we want to cache the 2 values in sync which leads to a MRUCache<GURL, pair<bool, string>> construct with special casing for string.empty() -> absent. This just seemed cleaner to me, any thoughts? https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... File components/web_restriction/web_restriction_provider.h (right): https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/web_restriction_provider.h:14: enum UrlAccess { On 2016/01/26 11:09:03, Bernhard Bauer wrote: > Move this into the class? This is used in other places as well like the resource throttle and interstitial as well as provider sub-classes. Isn't web_restriction namespace good enough?
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1631603002/diff/40001/components/web_restrict... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/40001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:50: url_access_cache[url_] = should_proceed; What happens when the parent grants permission? Do you clear this when the content provider notifies you of a change? https://codereview.chromium.org/1631603002/diff/40001/components/web_restrict... File components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionProvider.java (right): https://codereview.chromium.org/1631603002/diff/40001/components/web_restrict... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionProvider.java:65: Cursor result = mContentResolver.query(mQueryUri, null, select, null, null); This will block, potentially for some significant time, so must not be called on the UI or IO thread. It isn't clear to me whether it is better to move this to a different thread in C++ or Java.
https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... File components/web_restriction/content_resolver_web_restriction_provider.cc (right): https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.cc:81: const std::string& content_provider_authority) { Could you also DCHECK(!initialized_)? https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.cc:92: error_page_cache_.clear(); You could just DCHECK that these are empty, right? https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.cc:103: return ALLOW; When would this actually happen? https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... File components/web_restriction/content_resolver_web_restriction_provider.h (right): https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:28: class SelfDeletingCallback { On 2016/01/26 14:37:43, knn wrote: > On 2016/01/26 11:09:03, Bernhard Bauer wrote: > > Why is this in the header? We should be able to move it to the implementation > > file into an anonymous namespace. > > We need this class to be declared before the jni header include in the cc file. > It is either this or have a separate h/cc file for the callback which in turn > doesn't make sense as it is very tightly coupled with the provider class and we > will have include ordering issues. Hm. What we really need is just a declaration of the JNI method in the header, and that could live in the ContentResolverWebRestrictionProvider class and forward to the callback. Basically, I'd like to avoid having this class itself in the header, because it shows up in the namespace for anyone who includes this header. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:51: ~SelfDeletingCallback(); On 2016/01/26 14:37:43, knn wrote: > On 2016/01/26 11:09:03, Bernhard Bauer wrote: > > Methods come before members. > > But this is a destructor and seems appropriate to put it with the constructor > below which in turn always shows up in the end. The macro below technically defines a constructor, but conceptually it is just a declaration that this class does not have a copy constructor 😃 The style guide does say that methods come before members, and it has an explicit exception for DISALLOW_COPY_AND_ASSIGN (https://google.github.io/styleguide/cppguide.html#Declaration_Order). https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:57: class ContentResolverWebRestrictionProvider : public WebRestrictionProvider { Why is this not in the android namespace, BTW? https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:89: std::map<GURL, bool> url_access_cache_; On 2016/01/26 14:37:43, knn wrote: > On 2016/01/26 11:09:03, Bernhard Bauer wrote: > > How are we dealing with cache invalidation here? Also, you could use > > base/containers/mru_cache.h for this. > > We aren't. I was banking on a webview app's lifetime being limited usually. Hm, I could see various instances where that isn't the case, and then we get bug reports filed because OMG the filter doesn't work 😕 Do we support listening for changes to the filter? ContentProvider does offer that. We could also land this without caching as a first step, and then add caching when we have figured out how to invalidate it (e.g. by listening for changes). > Reason for not using MRUCache was we want to cache the 2 values in sync which > leads to a MRUCache<GURL, pair<bool, string>> construct with special casing for > string.empty() -> absent. This just seemed cleaner to me, any thoughts? Could we just use two MRU caches? https://codereview.chromium.org/1631603002/diff/40001/components/web_restrict... File components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionProvider.java (right): https://codereview.chromium.org/1631603002/diff/40001/components/web_restrict... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionProvider.java:65: Cursor result = mContentResolver.query(mQueryUri, null, select, null, null); On 2016/01/27 14:16:42, aberent wrote: > This will block, potentially for some significant time, so must not be called on > the UI or IO thread. It isn't clear to me whether it is better to move this to a > different thread in C++ or Java. Good catch! We can probably do this in a Java background thread and then directly call through to native code (i.e. without jumping back to the origin thread, which doesn't have a Looper because it was started from native). The native code in the callback does have a TaskRunner which it can use to go back to the origin.
PTAL. Thanks! https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... File components/web_restriction/content_resolver_web_restriction_provider.cc (right): https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.cc:81: const std::string& content_provider_authority) { On 2016/01/27 14:24:00, Bernhard Bauer wrote: > Could you also DCHECK(!initialized_)? I was thinking, this may be re-initialized with a new provider if the policy ever changes. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.cc:92: error_page_cache_.clear(); On 2016/01/27 14:24:01, Bernhard Bauer wrote: > You could just DCHECK that these are empty, right? See above. (provider change) https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.cc:103: return ALLOW; On 2016/01/27 14:24:00, Bernhard Bauer wrote: > When would this actually happen? If the policy is not set, we won't initialize it. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... File components/web_restriction/content_resolver_web_restriction_provider.h (right): https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:28: class SelfDeletingCallback { On 2016/01/27 14:24:01, Bernhard Bauer wrote: > On 2016/01/26 14:37:43, knn wrote: > > On 2016/01/26 11:09:03, Bernhard Bauer wrote: > > > Why is this in the header? We should be able to move it to the > implementation > > > file into an anonymous namespace. > > > > We need this class to be declared before the jni header include in the cc > file. > > It is either this or have a separate h/cc file for the callback which in turn > > doesn't make sense as it is very tightly coupled with the provider class and > we > > will have include ordering issues. > > Hm. What we really need is just a declaration of the JNI method in the header, > and that could live in the ContentResolverWebRestrictionProvider class and > forward to the callback. Basically, I'd like to avoid having this class itself > in the header, because it shows up in the namespace for anyone who includes this > header. Resolved the issue by casting to the callback myself instead of letting JNI do it. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:51: ~SelfDeletingCallback(); On 2016/01/27 14:24:01, Bernhard Bauer wrote: > On 2016/01/26 14:37:43, knn wrote: > > On 2016/01/26 11:09:03, Bernhard Bauer wrote: > > > Methods come before members. > > > > But this is a destructor and seems appropriate to put it with the constructor > > below which in turn always shows up in the end. > > The macro below technically defines a constructor, but conceptually it is just a > declaration that this class does not have a copy constructor 😃 The style guide > does say that methods come before members, and it has an explicit exception for > DISALLOW_COPY_AND_ASSIGN > (https://google.github.io/styleguide/cppguide.html#Declaration_Order). Done. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:57: class ContentResolverWebRestrictionProvider : public WebRestrictionProvider { On 2016/01/27 14:24:01, Bernhard Bauer wrote: > Why is this not in the android namespace, BTW? web_restrictions is implicitly android at the moment, got rid of the android namespace for the callback. https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.h:89: std::map<GURL, bool> url_access_cache_; On 2016/01/27 14:24:01, Bernhard Bauer wrote: > On 2016/01/26 14:37:43, knn wrote: > > On 2016/01/26 11:09:03, Bernhard Bauer wrote: > > > How are we dealing with cache invalidation here? Also, you could use > > > base/containers/mru_cache.h for this. > > > > We aren't. I was banking on a webview app's lifetime being limited usually. > > Hm, I could see various instances where that isn't the case, and then we get bug > reports filed because OMG the filter doesn't work 😕 > > Do we support listening for changes to the filter? ContentProvider does offer > that. We could also land this without caching as a first step, and then add > caching when we have figured out how to invalidate it (e.g. by listening for > changes). > > > Reason for not using MRUCache was we want to cache the 2 values in sync which > > leads to a MRUCache<GURL, pair<bool, string>> construct with special casing > for > > string.empty() -> absent. This just seemed cleaner to me, any thoughts? > > Could we just use two MRU caches? Clearing the cache on update now. Hope the cache helper methods look good. https://codereview.chromium.org/1631603002/diff/40001/components/web_restrict... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/40001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:50: url_access_cache[url_] = should_proceed; On 2016/01/27 14:16:42, aberent wrote: > What happens when the parent grants permission? Do you clear this when the > content provider notifies you of a change? Fixed this.
https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... File components/web_restriction/content_resolver_web_restriction_provider.cc (right): https://codereview.chromium.org/1631603002/diff/1/components/web_restriction/... components/web_restriction/content_resolver_web_restriction_provider.cc:81: const std::string& content_provider_authority) { On 2016/01/28 11:43:25, knn wrote: > On 2016/01/27 14:24:00, Bernhard Bauer wrote: > > Could you also DCHECK(!initialized_)? > > I was thinking, this may be re-initialized with a new provider if the policy > ever changes. That makes sense, but maybe add a comment that explicitly explains that we allow multiple initialization? Or actually, just call it SetAuthority()? https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:19: namespace cache_helper { I don't think this namespace is necessary, as all of this is in an anonymous namespace anyway. https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:24: // Move the url to the front of the cache. Nit: URL https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:34: const base::android::JavaParamRef<jstring>& j_error_page) { Use JavaRef (JavaParamRef is only a temporary thing used for JNI calls). https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:115: cache_helper::InsertValue( Just make this a method on |provider_|? That would also allow you to use a SequenceChecker to avoid simultaneously accessing the cache on multiple threads at the same time. https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:169: new SelfDeletingCallback(url, callback, callback_runner, this); I was going to write, "please mention that this deletes itself", but "SelfDeletingCallback" actually does a pretty good job of explaining that. Nice! 😃 Actually... Do we even need to allocate this object on the heap here and have it self-destruct? On the background thread, everything will happen synchronously, so we could just allocate it on the stack there, and it will go out of scope at the end of the method, *after* we have received the result. https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:170: content::BrowserThread::GetBlockingPool()->PostWorkerTaskWithShutdownBehavior( Speaking of multiple threads, you want to use a SequencedTaskRunner from the blocking pool here (or post the task with a sequence token), otherwise several of these calls might run at the same time and cause a race condition with the cache. https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... File components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java (right): https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:40: mContentResolver.registerContentObserver(baseUri, true, new ContentObserver(null) { We need to make sure we unregister this content observer when the native object goes away. https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:47: public void onChange(boolean selfChange, Uri uri) { BTW, on which thread is this called? We might again have to make sure we access the cache on the right thread.
No additional comments beyond Bernhard's comments.
PTAL. Hopefully the threading is right this time. Thanks! https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:19: namespace cache_helper { On 2016/01/28 12:15:14, Bernhard Bauer wrote: > I don't think this namespace is necessary, as all of this is in an anonymous > namespace anyway. More for structure but redundant now, I have made it a provider member. https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:24: // Move the url to the front of the cache. On 2016/01/28 12:15:14, Bernhard Bauer wrote: > Nit: URL Done. https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:34: const base::android::JavaParamRef<jstring>& j_error_page) { On 2016/01/28 12:15:14, Bernhard Bauer wrote: > Use JavaRef (JavaParamRef is only a temporary thing used for JNI calls). Good catch! https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:115: cache_helper::InsertValue( On 2016/01/28 12:15:14, Bernhard Bauer wrote: > Just make this a method on |provider_|? That would also allow you to use a > SequenceChecker to avoid simultaneously accessing the cache on multiple threads > at the same time. Done. https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:169: new SelfDeletingCallback(url, callback, callback_runner, this); On 2016/01/28 12:15:14, Bernhard Bauer wrote: > I was going to write, "please mention that this deletes itself", but > "SelfDeletingCallback" actually does a pretty good job of explaining that. Nice! > 😃 > > Actually... Do we even need to allocate this object on the heap here and have it > self-destruct? On the background thread, everything will happen synchronously, > so we could just allocate it on the stack there, and it will go out of scope at > the end of the method, *after* we have received the result. Indeed we do not need to make it self deleting given how it is presently but I am a bit worried it will make it very fragile incase we make it async say in some edge case. WDYT? https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/content_resolver_web_restrictions_provider.cc:170: content::BrowserThread::GetBlockingPool()->PostWorkerTaskWithShutdownBehavior( On 2016/01/28 12:15:14, Bernhard Bauer wrote: > Speaking of multiple threads, you want to use a SequencedTaskRunner from the > blocking pool here (or post the task with a sequence token), otherwise several > of these calls might run at the same time and cause a race condition with the > cache. Done. https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... File components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java (right): https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:40: mContentResolver.registerContentObserver(baseUri, true, new ContentObserver(null) { On 2016/01/28 12:15:14, Bernhard Bauer wrote: > We need to make sure we unregister this content observer when the native object > goes away. Done. https://codereview.chromium.org/1631603002/diff/80001/components/web_restrict... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:47: public void onChange(boolean selfChange, Uri uri) { On 2016/01/28 12:15:14, Bernhard Bauer wrote: > BTW, on which thread is this called? We might again have to make sure we access > the cache on the right thread. Right! Let's handle that on the native side.
Looks like I forgot to say PTAL!
You didn't, I just forgot to send this off. Sorry! https://codereview.chromium.org/1631603002/diff/100001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/100001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:88: provider_->UpdateCache(url_, should_proceed, error_page); We call this on the background thread, but we read from the cache on the origin thread, which is not going to work. Can we just post a task back to the origin thread as well to do this? (Then the background task runner shouldn't really be called |cache_update_task_runner_| anymore). https://codereview.chromium.org/1631603002/diff/100001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:121: Java_ContentResolverWebRestrictionsProvider_onDestroy(env, Call this in the destructor as well? https://codereview.chromium.org/1631603002/diff/100001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:232: // supports_request_ is initialized to false. Wrap this in |pipe symbols|?
PTAL. Thanks! https://codereview.chromium.org/1631603002/diff/100001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/100001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:88: provider_->UpdateCache(url_, should_proceed, error_page); On 2016/02/02 15:03:56, Bernhard Bauer wrote: > We call this on the background thread, but we read from the cache on the origin > thread, which is not going to work. Can we just post a task back to the origin > thread as well to do this? (Then the background task runner shouldn't really be > called |cache_update_task_runner_| anymore). Done. https://codereview.chromium.org/1631603002/diff/100001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:121: Java_ContentResolverWebRestrictionsProvider_onDestroy(env, On 2016/02/02 15:03:56, Bernhard Bauer wrote: > Call this in the destructor as well? Done. https://codereview.chromium.org/1631603002/diff/100001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:232: // supports_request_ is initialized to false. On 2016/02/02 15:03:56, Bernhard Bauer wrote: > Wrap this in |pipe symbols|? Done.
https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:78: const scoped_refptr<base::TaskRunner>& callback_runner, If the contract of ContentResolverWebRestrictionsProvider says it only works on the IO thread, you could just post all the tasks explicitly to the IO thread, so you wouldn't need this task runner. Or you could do it the other way around, and make the class usable on any thread (and then use a ThreadChecker or SequenceChecker to verify instead of DCHECK_CURRENTLY_ON). https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:98: base::Owned(error_page))); Could we wrap this in a scoped_ptr to make sure nothing leaks? It would also allow passing it further (see below). https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:245: error_page_cache_[url] = *error_page; Hm... We pass this as a pointer to this method to avoid copying the full page, right? Should we maybe store this in the cache as a scoped_ptr<std::string> then? (FTR, I would personally be fine with copying this around by value unless we see a clear indication of performance issues. Premature optimization and all.)
https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:97: base::Unretained(provider_), url_, should_proceed, Is |provider_| still guaranteed to still exist? Do we need to use a WeakPointer (in which case we have to be careful not to pass it between threads)? https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:287: base::Unretained(provider))); As above; do we know that |provider| hasn't been deleted (or won't be before the task runs)?
https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:245: error_page_cache_[url] = *error_page; On 2016/02/03 16:15:42, Bernhard Bauer wrote: > Hm... We pass this as a pointer to this method to avoid copying the full page, > right? Should we maybe store this in the cache as a scoped_ptr<std::string> > then? > > (FTR, I would personally be fine with copying this around by value unless we see > a clear indication of performance issues. Premature optimization and all.) I am using pointers mostly to indicate missing values. Are we also fine to assume empty string == no error page?
https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:97: base::Unretained(provider_), url_, should_proceed, On 2016/02/03 16:22:07, aberent wrote: > Is |provider_| still guaranteed to still exist? Do we need to use a WeakPointer > (in which case we have to be careful not to pass it between threads)? Yes, it is like a singleton should only be reset with SetAuthority, I'll add a comment to that effect.
https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:245: error_page_cache_[url] = *error_page; On 2016/02/03 16:23:56, knn wrote: > On 2016/02/03 16:15:42, Bernhard Bauer wrote: > > Hm... We pass this as a pointer to this method to avoid copying the full page, > > right? Should we maybe store this in the cache as a scoped_ptr<std::string> > > then? > > > > (FTR, I would personally be fine with copying this around by value unless we > see > > a clear indication of performance issues. Premature optimization and all.) > > I am using pointers mostly to indicate missing values. Are we also fine to > assume empty string == no error page? Yeah, I don't think anyone would legitimately want to use a completely empty string as the error page.
PTAL. Thanks! https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:78: const scoped_refptr<base::TaskRunner>& callback_runner, On 2016/02/03 16:15:42, Bernhard Bauer wrote: > If the contract of ContentResolverWebRestrictionsProvider says it only works on > the IO thread, you could just post all the tasks explicitly to the IO thread, so > you wouldn't need this task runner. Or you could do it the other way around, and > make the class usable on any thread (and then use a ThreadChecker or > SequenceChecker to verify instead of DCHECK_CURRENTLY_ON). Done. https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:98: base::Owned(error_page))); On 2016/02/03 16:15:42, Bernhard Bauer wrote: > Could we wrap this in a scoped_ptr to make sure nothing leaks? It would also > allow passing it further (see below). Passing by value now. https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:245: error_page_cache_[url] = *error_page; On 2016/02/03 16:26:49, Bernhard Bauer wrote: > On 2016/02/03 16:23:56, knn wrote: > > On 2016/02/03 16:15:42, Bernhard Bauer wrote: > > > Hm... We pass this as a pointer to this method to avoid copying the full > page, > > > right? Should we maybe store this in the cache as a scoped_ptr<std::string> > > > then? > > > > > > (FTR, I would personally be fine with copying this around by value unless we > > see > > > a clear indication of performance issues. Premature optimization and all.) > > > > I am using pointers mostly to indicate missing values. Are we also fine to > > assume empty string == no error page? > > Yeah, I don't think anyone would legitimately want to use a completely empty > string as the error page. Acknowledged.
https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:97: base::Unretained(provider_), url_, should_proceed, On 2016/02/03 16:26:07, knn wrote: > On 2016/02/03 16:22:07, aberent wrote: > > Is |provider_| still guaranteed to still exist? Do we need to use a > WeakPointer > > (in which case we have to be careful not to pass it between threads)? > > Yes, it is like a singleton should only be reset with SetAuthority, I'll add a > comment to that effect. I am confused. It is the lifetime of the C++ ContentResolverWebRestrictionsProvider object I am worried about, not the lifetime of the Java object. SetAuthority only controls the lifetime of the Java object, whereas even if the this method it called before the C++ object is deleted, the task runs some time later, possibly after the C++ object has been deleted. What controls the lifetime of the C++ object? I see nothing in this CL that guarantees that it can't be deleted. I think your Webview CL does guarantee that in a Webview context it will never be deleted, but that is very non-local, so error prone.
https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/120001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:97: base::Unretained(provider_), url_, should_proceed, On 2016/02/03 17:56:15, aberent wrote: > On 2016/02/03 16:26:07, knn wrote: > > On 2016/02/03 16:22:07, aberent wrote: > > > Is |provider_| still guaranteed to still exist? Do we need to use a > > WeakPointer > > > (in which case we have to be careful not to pass it between threads)? > > > > Yes, it is like a singleton should only be reset with SetAuthority, I'll add a > > comment to that effect. > > I am confused. It is the lifetime of the C++ > ContentResolverWebRestrictionsProvider object I am worried about, not the > lifetime of the Java object. SetAuthority only controls the lifetime of the Java > object, whereas even if the this method it called before the C++ object is > deleted, the task runs some time later, possibly after the C++ object has been > deleted. > > What controls the lifetime of the C++ object? I see nothing in this CL that > guarantees that it can't be deleted. I think your Webview CL does guarantee that > in a Webview context it will never be deleted, but that is very non-local, so > error prone. Yes, what you are saying is correct there are no *guarantees* that this object won't be created and destroyed at random. There should be 2 instances of this object in webview and chrome. My comment basically documents that. Further passing weak pointers to java looks like a tricky beast for NotifyWebRestrictionsChanged.
More threading problems. Sorry! https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:85: void SelfDeletingCallback::ShouldProceed(bool should_proceed, Further to my previous threading comments. I think this can get the result back after the provider authority has been switched by a call to SetAuthority. What should we do in that case? If we do nothing then the result from the old authority will be added to the new cache and used. https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:91: error_page)); Even if we are still using the same authority when the function it called, we may not be by the time this task runs. https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:92: task_runner_->PostTask(FROM_HERE, base::Bind(callback_, should_proceed)); And the authority could change between the first and second task running. https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... File components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java (right): https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:87: Cursor result = mContentResolver.query(mQueryUri, null, select, null, null); Does this need a null check and synchronization with onDestroy? They are, I think, called on different threads. https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:102: boolean requestSuccess = mContentResolver.insert(mRequestUri, values) != null; null check and synchronization?
PTAL. Thanks! https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:85: void SelfDeletingCallback::ShouldProceed(bool should_proceed, On 2016/02/03 19:26:22, aberent wrote: > Further to my previous threading comments. I think this can get the result back > after the provider authority has been switched by a call to SetAuthority. What > should we do in that case? If we do nothing then the result from the old > authority will be added to the new cache and used. Good catch! Fixed this now. https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:91: error_page)); On 2016/02/03 19:26:22, aberent wrote: > Even if we are still using the same authority when the function it called, we > may not be by the time this task runs. Fixed. https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:92: task_runner_->PostTask(FROM_HERE, base::Bind(callback_, should_proceed)); On 2016/02/03 19:26:22, aberent wrote: > And the authority could change between the first and second task running. So, we probably don't care about requests that were made when the old authority was set up. The user can expect the request to be the answered by the old one. I'd say WAI, as long as cache is intact for future requests. https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... File components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java (right): https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:87: Cursor result = mContentResolver.query(mQueryUri, null, select, null, null); On 2016/02/03 19:26:22, aberent wrote: > Does this need a null check and synchronization with onDestroy? They are, I > think, called on different threads. Fixed by ensuring destructor is only called on the correct thread. I guess this means the object can only be owned by a specific thread now. https://codereview.chromium.org/1631603002/diff/160001/components/web_restric... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:102: boolean requestSuccess = mContentResolver.insert(mRequestUri, values) != null; On 2016/02/03 19:26:22, aberent wrote: > null check and synchronization? Same as above.
lgtm - assuming we are doing the tests as a separate CL. https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... File components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java (right): https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:66: synchronized (this) { Nit - I don't think Findbugs will like synchronized (this), although I don't understand the problem with it. In general our synchronization code uses a special synchronization object rather than synchronized(this). See, for example, https://code.google.com/p/chromium/codesearch#chromium/src/sync/android/java/...
https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:234: if (provider_authority != provider_authority_) You could add a comment about why we do this. https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.h (right): https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.h:58: void UpdateCache(std::string provider_authority, Pass by const-ref. https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... File components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java (right): https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:90: synchronized (this) { Hm... wouldn't this potentially hold the lock for a long time? Then if we'd call onDestroy on the IO thread, it would block until this finishes. We could add an accessor for |mContentResolver| that is synchronized, and then only use it through that accessor, so that onDestroy() could still be called concurrently.
PTAL. Thanks! https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.cc (right): https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.cc:234: if (provider_authority != provider_authority_) On 2016/02/04 11:13:53, Bernhard Bauer wrote: > You could add a comment about why we do this. Feels a bit verbose to me but, Done. https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... File components/web_restrictions/content_resolver_web_restrictions_provider.h (right): https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... components/web_restrictions/content_resolver_web_restrictions_provider.h:58: void UpdateCache(std::string provider_authority, On 2016/02/04 11:13:53, Bernhard Bauer wrote: > Pass by const-ref. Passed in a callback from SelfDeletingCallback so the reference would be deleted. https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... File components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java (right): https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:66: synchronized (this) { On 2016/02/04 10:53:58, aberent wrote: > Nit - I don't think Findbugs will like synchronized (this), although I don't > understand the problem with it. In general our synchronization code uses a > special synchronization object rather than synchronized(this). > > See, for example, > https://code.google.com/p/chromium/codesearch#chromium/src/sync/android/java/... Removed. https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:90: synchronized (this) { On 2016/02/04 11:13:53, Bernhard Bauer wrote: > Hm... wouldn't this potentially hold the lock for a long time? Then if we'd call > onDestroy on the IO thread, it would block until this finishes. > > We could add an accessor for |mContentResolver| that is synchronized, and then > only use it through that accessor, so that onDestroy() could still be called > concurrently. We don't even need a synchronized, do we? Finally the GC is useful somewhere.
knn@chromium.org changed reviewers: + jochen@chromium.org, mef@chromium.org
Adding Owners for added DEPS, jochen@chromium.org: Please review a DEP on: content/public, gin, third_party/WebKit (using them in web_restrictions_gin_wrapper) mef@chromium.org: Please review a DEP on: net (using net/url_request/ in web_restrictions_resource_throttle)
Still lgtm
https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... File components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java (right): https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:90: synchronized (this) { On 2016/02/04 13:09:27, knn wrote: > On 2016/02/04 11:13:53, Bernhard Bauer wrote: > > Hm... wouldn't this potentially hold the lock for a long time? Then if we'd > call > > onDestroy on the IO thread, it would block until this finishes. > > > > We could add an accessor for |mContentResolver| that is synchronized, and then > > only use it through that accessor, so that onDestroy() could still be called > > concurrently. > > We don't even need a synchronized, do we? Finally the GC is useful somewhere. Hm, if you only ever read the content resolver, it might be fine, but nulling it out might not be propagated to other threads properly if you don't use synchronization. Maybe just remove that part? Then you could make the content resolver static as well.
PTAL. Thanks! https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... File components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java (right): https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:90: synchronized (this) { On 2016/02/04 14:27:21, Bernhard Bauer wrote: > On 2016/02/04 13:09:27, knn wrote: > > On 2016/02/04 11:13:53, Bernhard Bauer wrote: > > > Hm... wouldn't this potentially hold the lock for a long time? Then if we'd > > call > > > onDestroy on the IO thread, it would block until this finishes. > > > > > > We could add an accessor for |mContentResolver| that is synchronized, and > then > > > only use it through that accessor, so that onDestroy() could still be > called > > > concurrently. > > > > We don't even need a synchronized, do we? Finally the GC is useful somewhere. > > Hm, if you only ever read the content resolver, it might be fine, but nulling it > out might not be propagated to other threads properly if you don't use > synchronization. Maybe just remove that part? Then you could make the content > resolver static as well. I guess volatile was what I was looking for, given the syncing in the native side, I am happy to remove the nulling. BTW you meant final and not static right?
lgtm https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... File components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java (right): https://codereview.chromium.org/1631603002/diff/200001/components/web_restric... components/web_restrictions/java/src/org/chromium/components/webrestrictions/ContentResolverWebRestrictionsProvider.java:90: synchronized (this) { On 2016/02/04 15:31:33, knn wrote: > On 2016/02/04 14:27:21, Bernhard Bauer wrote: > > On 2016/02/04 13:09:27, knn wrote: > > > On 2016/02/04 11:13:53, Bernhard Bauer wrote: > > > > Hm... wouldn't this potentially hold the lock for a long time? Then if > we'd > > > call > > > > onDestroy on the IO thread, it would block until this finishes. > > > > > > > > We could add an accessor for |mContentResolver| that is synchronized, and > > then > > > > only use it through that accessor, so that onDestroy() could still be > > called > > > > concurrently. > > > > > > We don't even need a synchronized, do we? Finally the GC is useful > somewhere. > > > > Hm, if you only ever read the content resolver, it might be fine, but nulling > it > > out might not be propagated to other threads properly if you don't use > > synchronization. Maybe just remove that part? Then you could make the content > > resolver static as well. > > I guess volatile was what I was looking for, given the syncing in the native > side, I am happy to remove the nulling. > BTW you meant final and not static right? Urr, yes.
https://codereview.chromium.org/1631603002/diff/240001/components/web_restric... File components/web_restrictions/DEPS (right): https://codereview.chromium.org/1631603002/diff/240001/components/web_restric... components/web_restrictions/DEPS:2: "+content/public", please don't mix browser and renderer stuff in one target. Instead, create the usual subdirectory structure (browser/common/renderer) and create individual targets for that. It would also be great to include the code that uses this component in the CL, so it's easier to see whether the interface is right. last but not least: tests please https://codereview.chromium.org/1631603002/diff/240001/components/web_restric... File components/web_restrictions/web_restrictions_gin_wrapper.h (right): https://codereview.chromium.org/1631603002/diff/240001/components/web_restric... components/web_restrictions/web_restrictions_gin_wrapper.h:36: content::RenderFrame* render_frame_; please derive from RenderFrameObserver instead of maintaining the pointer yourself
knn@ - I cannot wait any longer for this; since you are no longer on the team I am taking it over. Please discuss with me before landing any further changes.
not lgtm Cancelling LGTM since this is now replaced by https://codereview.chromium.org/1684153002
Description was changed from ========== Web restrictions component only. Splitting away non webview specific pieces from https://codereview.chromium.org/1423713015 so that Anthony can work parallely on the chrome bits while the webview bits are reviewed. The individual pieces are: A content resolver based web restriction provider. A gin wrapper and a resource throttle to tie it together. BUG=569879 ========== to ========== Web restrictions component only. Splitting away non webview specific pieces from https://codereview.chromium.org/1423713015 so that Anthony can work parallely on the chrome bits while the webview bits are reviewed. The individual pieces are: A content resolver based web restriction provider. A gin wrapper and a resource throttle to tie it together. BUG=569879 ==========
Message was sent while issue was closed.
https://codereview.chromium.org/1631603002/diff/240001/components/web_restric... File components/web_restrictions/DEPS (right): https://codereview.chromium.org/1631603002/diff/240001/components/web_restric... components/web_restrictions/DEPS:2: "+content/public", On 2016/02/05 14:55:07, jochen (OOO) wrote: > please don't mix browser and renderer stuff in one target. > > Instead, create the usual subdirectory structure (browser/common/renderer) and > create individual targets for that. Done in https://codereview.chromium.org/1684153002/ > > It would also be great to include the code that uses this component in the CL, > so it's easier to see whether the interface is right. I would prefer to keep this separate, because if I include this, and the tests, the CL becomes unmanageably large. You can see an initial version of its use in https://codereview.chromium.org/1423713015. > > last but not least: tests please Done in https://codereview.chromium.org/1684153002/ https://codereview.chromium.org/1631603002/diff/240001/components/web_restric... File components/web_restrictions/web_restrictions_gin_wrapper.h (right): https://codereview.chromium.org/1631603002/diff/240001/components/web_restric... components/web_restrictions/web_restrictions_gin_wrapper.h:36: content::RenderFrame* render_frame_; On 2016/02/05 14:55:07, jochen (OOO) wrote: > please derive from RenderFrameObserver instead of maintaining the pointer > yourself Done in https://codereview.chromium.org/1684153002/ |
