Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1393073002:
Add external data use observer (Closed)
|
Created:
5 years, 2 months ago by tbansal1 Modified:
5 years, 2 months ago CC:
blundell+watchlist_chromium.org, cbentzel+watch_chromium.org, chromium-reviews, droger+watchlist_chromium.org, sdefresne+watchlist_chromium.org, bengr Base URL:
https://chromium.googlesource.com/chromium/src.git@datause_accounting_scliitle_cl_do_not_edit_2 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd external data use observer that will listen to data use
observations and pass them to Java over JNI.
ExternalDataUseObserver is created and owned by IOThread.
ExternalDataUseObserver listens to data use observations,
matches them with the regexes and passes them over to a
Java object over JNI. The Java object is of type
ExternalDataUseObserverAndroid.
For design doc, please see the bug.
This CL only has upstream implementation of
ExternalDataUseObserverAndroid.java which really does not
do much. Downstream implementation of
ExternalDataUseObserverAndroid.java will come in a separate
downstream CL. That would do the heavylifting of talking
with external entities as per the design doc.
BUG=540061
Committed: https://crrev.com/796988c4f597a0942455522351e5f2703ccea95d
Cr-Commit-Position: refs/heads/master@{#353950}
Patch Set 1 : Updated #
Total comments: 10
Patch Set 2 : More tests #
Total comments: 26
Patch Set 3 : Addressed comments #
Total comments: 6
Patch Set 4 : Addressed comments, removed android suffix #
Total comments: 2
Patch Set 5 : Addressed sclittle comment #
Total comments: 4
Patch Set 6 : onDestroy added #
Total comments: 2
Patch Set 7 : Rebased, newt's comment addressed #Patch Set 8 : assert added #Messages
Total messages: 40 (14 generated)
Patchset #1 (id:1) has been deleted
tbansal@chromium.org changed reviewers: + bengr@chromium.org, sclittle@chromium.org
Sanity check. PTAL at patch set 2 vs. patch set 1 (patch set 1 is the parent branch from yet to be submitted CL).
drive-by: Does the data_usage component really need to be a layered component (http://www.chromium.org/developers/design-documents/layered-components-design)? This is required when the component is used on iOS and needs to have dependencies to src/content vs. src/ios/web (on iOS). If it does not needs to be a layered component, maybe you can remove the components/data_usage/core sub-directory and put everything into components/data_usage.
Patchset #3 (id:60001) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:80001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #1 (id:100001) has been deleted
sclittle, bengr: I have now rebased this CL. PTAL. @sdefresne: The code that you commented on is now gone :)
https://codereview.chromium.org/1393073002/diff/120001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.cc (right): https://codereview.chromium.org/1393073002/diff/120001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.cc:49: std::string gurl_spec = gurl.spec(); nit: To avoid the unnecessary string allocation, change to: const std::string& gurl_spec = gurl.spec(); https://codereview.chromium.org/1393073002/diff/120001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.cc:73: Java_ExternalDataUseObserverAndroid_onDataUse( Hmmm - as the aggregator exists right now, this will call into JNI on every read and write and block the request processing. Even once the DataUseAggregator starts buffering and amortizing DataUse, it could still happen very often. Could the Java ExternalDataUseObserver specify some buffering parameters or something for this C++ observer to follow? Like, perhaps to report once every K bytes or something. Also, could it be possible to run the JNI stuff on a different thread or something, so that it doesn't block request processing? Or do we want it to be possible for the JNI stuff to be able to block request processing? https://codereview.chromium.org/1393073002/diff/120001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.h (right): https://codereview.chromium.org/1393073002/diff/120001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:52: // Aggregator that sends data use observations. May be nullptr. Why can this be nullptr? https://codereview.chromium.org/1393073002/diff/120001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:59: std::vector<std::string> url_regexes_; Could you pre-compile the RE2 patterns? https://codereview.chromium.org/1393073002/diff/120001/components/data_usage/... File components/data_usage/core/data_use_aggregator.h (right): https://codereview.chromium.org/1393073002/diff/120001/components/data_usage/... components/data_usage/core/data_use_aggregator.h:63: scoped_ptr<Observer> external_observer_; Making the DataUseAggregator own one of its observers actually gets quite weird, especially around destruction time. The Observer's destructor (which calls data_use_aggregator_->RemoveObserver) will run after the DataUseAggregator's destructor, which will probably explode.
Patchset #2 (id:160001) has been deleted
Patchset #2 (id:180001) has been deleted
tbansal@chromium.org changed reviewers: + mmenke@chromium.org, newt@chromium.org
mmenke: PTAL at io_thread.* newt: everything else :) Thanks. https://codereview.chromium.org/1393073002/diff/120001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.cc (right): https://codereview.chromium.org/1393073002/diff/120001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.cc:49: std::string gurl_spec = gurl.spec(); On 2015/10/09 22:59:28, sclittle wrote: > nit: To avoid the unnecessary string allocation, change to: > > const std::string& gurl_spec = gurl.spec(); Done. https://codereview.chromium.org/1393073002/diff/120001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.cc:73: Java_ExternalDataUseObserverAndroid_onDataUse( On 2015/10/09 22:59:28, sclittle wrote: > Hmmm - as the aggregator exists right now, this will call into JNI on every read > and write and block the request processing. > > Even once the DataUseAggregator starts buffering and amortizing DataUse, it > could still happen very often. Could the Java ExternalDataUseObserver specify > some buffering parameters or something for this C++ observer to follow? Like, > perhaps to report once every K bytes or something. > > Also, could it be possible to run the JNI stuff on a different thread or > something, so that it doesn't block request processing? Or do we want it to be > possible for the JNI stuff to be able to block request processing? Added TODO for buffering. Plan is to report every K bytes. https://codereview.chromium.org/1393073002/diff/120001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.h (right): https://codereview.chromium.org/1393073002/diff/120001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:52: // Aggregator that sends data use observations. May be nullptr. On 2015/10/09 22:59:28, sclittle wrote: > Why can this be nullptr? Removed. https://codereview.chromium.org/1393073002/diff/120001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:59: std::vector<std::string> url_regexes_; On 2015/10/09 22:59:28, sclittle wrote: > Could you pre-compile the RE2 patterns? Done. https://codereview.chromium.org/1393073002/diff/120001/components/data_usage/... File components/data_usage/core/data_use_aggregator.h (right): https://codereview.chromium.org/1393073002/diff/120001/components/data_usage/... components/data_usage/core/data_use_aggregator.h:63: scoped_ptr<Observer> external_observer_; On 2015/10/09 22:59:28, sclittle wrote: > Making the DataUseAggregator own one of its observers actually gets quite weird, > especially around destruction time. The Observer's destructor (which calls > data_use_aggregator_->RemoveObserver) will run after the DataUseAggregator's > destructor, which will probably explode. changed the owner to IO thread globals.
io_thread LGTM.
https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.h:131: external_data_use_observer; Actually, can this just be in IOThread? If you need these to have one of the properties of globals (Settable for testing, used in InitFromGlobals, exposed publicly), that's fine, though I'd like to know which one it is.
https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.h:131: external_data_use_observer; On 2015/10/12 16:59:26, mmenke wrote: > Actually, can this just be in IOThread? If you need these to have one of the > properties of globals (Settable for testing, used in InitFromGlobals, exposed > publicly), that's fine, though I'd like to know which one it is. No, I do not think I need any of those properties. It seems that |data_use_aggregator| can also move to IOThread? I just wanted to keep them together. wdyt?
On 2015/10/12 17:05:18, tbansal1 wrote: > https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/io_thre... > File chrome/browser/io_thread.h (right): > > https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/io_thre... > chrome/browser/io_thread.h:131: external_data_use_observer; > On 2015/10/12 16:59:26, mmenke wrote: > > Actually, can this just be in IOThread? If you need these to have one of the > > properties of globals (Settable for testing, used in InitFromGlobals, exposed > > publicly), that's fine, though I'd like to know which one it is. > > No, I do not think I need any of those properties. It seems that > |data_use_aggregator| can also move to IOThread? I just wanted to keep them > together. wdyt? SGTM
I have some general and Android-y comments. Someone else should review the details of ExternalDataUserObserver, though, as I barely looked at the C++ side of that code. https://codereview.chromium.org/1393073002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java (right): https://codereview.chromium.org/1393073002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java:16: * Chromium. This class is not thread safe. Based on the commit message, this class will only be used on the IO thread. This should be clearly documented, since it differs from the vast majority of Java classes in src/chrome, which are used only on the UI thread. (Also, if there's not a good reason for keeping this on the IO thread, moving this to run on the UI thread would reduce the likelihood of bugs.) https://codereview.chromium.org/1393073002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java:21: private static NonThreadSafe sThreadCheck = null; Most classes in Chrome aren't thread safe, and we usually don't add thread checks. If the class has a tricky or non-standard threading situation, then it's reasonable to add thread checks. https://codereview.chromium.org/1393073002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java:22: private static ExternalDataUseObserverAndroid sExternalDataUseObserverAndroid; Avoid statics, especially if they contain non-trivial member variables. A static object lives for the entire lifetime of the JVM, so it can (and will) outlive the browsing session, the profile, etc. If the static object happens to have a (direct or indirect) reference to an activity or profile or window etc, that will cause a serious memory leak. https://codereview.chromium.org/1393073002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java:48: public ExternalDataUseObserverAndroid() { looks like this should be protected https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.h (right): https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. Since this class is already inside an "android" folder and since there's no equivalent of this class for other platforms besides Android, it doesn't need the "_android" suffix at the end of the filename or class name. https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:28: // This class defines a data use observer external to Chromium. It creates and This comment should be much clearer. Imagine that you're explaining this class/feature to someone who hasn't read the design doc, and give an overview of what this class does / why it exists, before you dive into implementation details. In particular, after reading this comment, I have several (major) questions: - What does "external to Chromium" mean? Is this a way that other apps can observe Chromium's data usage, or that Chromium can observe other app's data usage? This is a critical distinction. - What are these regular expressions used for? Are they matched against app names, or app IDs, or web address, or what? If the use of these regexes is already explained in DataUseAggregator, then you could just say, "see DataUseAggregator::FooRegexMethod()". Here's how the comment might read: "This class allows C++ classes to observe how much data is used by other apps (besides Chromium) on the current Android device. It accepts a regex that allows for filtering data usage by URLs that match the regex. ... etc"
https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.cc (right): https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.cc:91: options.set_case_sensitive(false); nit: Does it have to be case insensitive? I think URL hostnames are case-insensitive, but other stuff in the URL is not (e.g. base64-encoded URL parameters?). For example, https://www.google.com/search?q=chromium vs. https://www.google.com/search?q=CHROMIUM. https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.cc:92: scoped_ptr<re2::RE2> pattern(new re2::RE2(url_regex, options)); What if the pattern is an invalid regex? https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.h (right): https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:16: #include "cc/base/scoped_ptr_vector.h" Why "cc/base/scoped_ptr_vector.h" instead of "base/memory/scoped_vector.h"?
bengr@chromium.org changed reviewers: - bengr@chromium.org
ptal. thanks. https://codereview.chromium.org/1393073002/diff/200001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java (right): https://codereview.chromium.org/1393073002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java:16: * Chromium. This class is not thread safe. On 2015/10/12 18:01:18, newt wrote: > Based on the commit message, this class will only be used on the IO thread. This > should be clearly documented, since it differs from the vast majority of Java > classes in src/chrome, which are used only on the UI thread. (Also, if there's > not a good reason for keeping this on the IO thread, moving this to run on the > UI thread would reduce the likelihood of bugs.) It is easier if we keep it on IO thread since the native code will also be on IO thread. Added comment. https://codereview.chromium.org/1393073002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java:21: private static NonThreadSafe sThreadCheck = null; On 2015/10/12 18:01:18, newt wrote: > Most classes in Chrome aren't thread safe, and we usually don't add thread > checks. If the class has a tricky or non-standard threading situation, then it's > reasonable to add thread checks. Removed. https://codereview.chromium.org/1393073002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java:22: private static ExternalDataUseObserverAndroid sExternalDataUseObserverAndroid; On 2015/10/12 18:01:18, newt wrote: > Avoid statics, especially if they contain non-trivial member variables. A static > object lives for the entire lifetime of the JVM, so it can (and will) outlive > the browsing session, the profile, etc. If the static object happens to have a > (direct or indirect) reference to an activity or profile or window etc, that > will cause a serious memory leak. Removed. https://codereview.chromium.org/1393073002/diff/200001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java:48: public ExternalDataUseObserverAndroid() { On 2015/10/12 18:01:18, newt wrote: > looks like this should be protected It complains because ChromeApplication is unable to access it. https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.cc (right): https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.cc:91: options.set_case_sensitive(false); On 2015/10/12 20:10:15, sclittle wrote: > nit: Does it have to be case insensitive? I think URL hostnames are > case-insensitive, but other stuff in the URL is not (e.g. base64-encoded URL > parameters?). For example, https://www.google.com/search?q=chromium vs. > https://www.google.com/search?q=CHROMIUM. yes, right now the regex support case insensitive matching only. In case, we need to support, case sensitive matching, we can add JNI calls where the external observer tells us if the supplied regex should be considered as case sensitive or not. https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.cc:92: scoped_ptr<re2::RE2> pattern(new re2::RE2(url_regex, options)); On 2015/10/12 20:10:15, sclittle wrote: > What if the pattern is an invalid regex? Done. https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.h (right): https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/10/12 18:01:18, newt wrote: > Since this class is already inside an "android" folder and since there's no > equivalent of this class for other platforms besides Android, it doesn't need > the "_android" suffix at the end of the filename or class name. In the past, I did this and later had to add a base class which required too much code churn and renaming of classes. Is it okay if I keep the "android" suffix? or, is it totally against the conventions? https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:16: #include "cc/base/scoped_ptr_vector.h" On 2015/10/12 20:10:15, sclittle wrote: > Why "cc/base/scoped_ptr_vector.h" instead of "base/memory/scoped_vector.h"? scoped_vector requires creating raw pointers and then pushing them on the ScopedVector object. However, if I create raw pointers, and later it turns out that the pattern is invalid, then I can't push it on the ScopedVector and have to manually delete the raw pointer. I think ScopedPtrVector better suits our purpose since scoped_ptr will automatically delete. https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:28: // This class defines a data use observer external to Chromium. It creates and On 2015/10/12 18:01:18, newt wrote: > This comment should be much clearer. Imagine that you're explaining this > class/feature to someone who hasn't read the design doc, and give an overview of > what this class does / why it exists, before you dive into implementation > details. > > In particular, after reading this comment, I have several (major) questions: > - What does "external to Chromium" mean? Is this a way that other apps can > observe Chromium's data usage, or that Chromium can observe other app's data > usage? This is a critical distinction. > - What are these regular expressions used for? Are they matched against app > names, or app IDs, or web address, or what? If the use of these regexes is > already explained in DataUseAggregator, then you could just say, "see > DataUseAggregator::FooRegexMethod()". > > Here's how the comment might read: "This class allows C++ classes to observe how > much data is used by other apps (besides Chromium) on the current Android > device. It accepts a regex that allows for filtering data usage by URLs that > match the regex. ... etc" Added more documentation. https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/io_thre... File chrome/browser/io_thread.h (right): https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.h:131: external_data_use_observer; On 2015/10/12 17:05:18, tbansal1 wrote: > On 2015/10/12 16:59:26, mmenke wrote: > > Actually, can this just be in IOThread? If you need these to have one of the > > properties of globals (Settable for testing, used in InitFromGlobals, exposed > > publicly), that's fine, though I'd like to know which one it is. > > No, I do not think I need any of those properties. It seems that > |data_use_aggregator| can also move to IOThread? I just wanted to keep them > together. wdyt? Done. https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/io_thre... chrome/browser/io_thread.h:131: external_data_use_observer; On 2015/10/12 16:59:26, mmenke wrote: > Actually, can this just be in IOThread? If you need these to have one of the > properties of globals (Settable for testing, used in InitFromGlobals, exposed > publicly), that's fine, though I'd like to know which one it is. Done.
Could you add me to the internal side of this review so I can see how the upstream/downstream parts interact? https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.h (right): https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/10/12 22:39:14, tbansal1 wrote: > On 2015/10/12 18:01:18, newt wrote: > > Since this class is already inside an "android" folder and since there's no > > equivalent of this class for other platforms besides Android, it doesn't need > > the "_android" suffix at the end of the filename or class name. > > In the past, I did this and later had to add a base class which required > too much code churn and renaming of classes. Is it okay if I keep the > "android" suffix? or, is it totally against the conventions? If it seems likely that you'll need to add a base class or platform-independent class, then it's OK to use the "_android" suffix, but otherwise I'd avoid it. https://codereview.chromium.org/1393073002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java (right): https://codereview.chromium.org/1393073002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java:28: * Creates an instance of |@link #sExternalDataUseObserverAndroid}. This comment isn't correct https://codereview.chromium.org/1393073002/diff/220001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.cc (right): https://codereview.chromium.org/1393073002/diff/220001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.cc:24: DCHECK(j_external_data_use_observer_.is_null()); This DCHECK is superfluous. Of course the member variable hasn't been initialized yet -- we're still at the beginning of the constructor.
https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.h (right): https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:16: #include "cc/base/scoped_ptr_vector.h" On 2015/10/12 22:39:14, tbansal1 wrote: > On 2015/10/12 20:10:15, sclittle wrote: > > Why "cc/base/scoped_ptr_vector.h" instead of "base/memory/scoped_vector.h"? > > scoped_vector requires creating raw pointers and then pushing > them on the ScopedVector object. > > However, if I create raw pointers, and later it turns out that the pattern is > invalid, > then I can't push it on the ScopedVector and have to manually delete the raw > pointer. > > I think ScopedPtrVector better suits our purpose since scoped_ptr will > automatically delete. ScopedVectors can also take scoped_ptrs: https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... I'd recommend using base/memory/scoped_vector.h over cc/base/scoped_ptr_vector.h - I've never seen this "cc/" folder used, and I don't know what is allowed to depend on it, or if it exists on all platforms. https://codereview.chromium.org/1393073002/diff/220001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.cc (right): https://codereview.chromium.org/1393073002/diff/220001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.cc:43: for (size_t i = 0; i < url_patterns_.size(); ++i) { nit: If you change |url_patterns_| to a ScopedVector, you can just use a range-based for loop here.
ptal. thanks. https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.h (right): https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/10/13 19:05:20, newt wrote: > On 2015/10/12 22:39:14, tbansal1 wrote: > > On 2015/10/12 18:01:18, newt wrote: > > > Since this class is already inside an "android" folder and since there's no > > > equivalent of this class for other platforms besides Android, it doesn't > need > > > the "_android" suffix at the end of the filename or class name. > > > > In the past, I did this and later had to add a base class which required > > too much code churn and renaming of classes. Is it okay if I keep the > > "android" suffix? or, is it totally against the conventions? > > If it seems likely that you'll need to add a base class or platform-independent > class, then it's OK to use the "_android" suffix, but otherwise I'd avoid it. OK, for now I don't forecast any platform independent class. So, removing the suffix. https://codereview.chromium.org/1393073002/diff/200001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.h:16: #include "cc/base/scoped_ptr_vector.h" On 2015/10/13 19:46:56, sclittle wrote: > On 2015/10/12 22:39:14, tbansal1 wrote: > > On 2015/10/12 20:10:15, sclittle wrote: > > > Why "cc/base/scoped_ptr_vector.h" instead of "base/memory/scoped_vector.h"? > > > > scoped_vector requires creating raw pointers and then pushing > > them on the ScopedVector object. > > > > However, if I create raw pointers, and later it turns out that the pattern is > > invalid, > > then I can't push it on the ScopedVector and have to manually delete the raw > > pointer. > > > > I think ScopedPtrVector better suits our purpose since scoped_ptr will > > automatically delete. > > ScopedVectors can also take scoped_ptrs: > https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/scoped... > > I'd recommend using base/memory/scoped_vector.h over cc/base/scoped_ptr_vector.h > - I've never seen this "cc/" folder used, and I don't know what is allowed to > depend on it, or if it exists on all platforms. Done. https://codereview.chromium.org/1393073002/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java (right): https://codereview.chromium.org/1393073002/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserverAndroid.java:28: * Creates an instance of |@link #sExternalDataUseObserverAndroid}. On 2015/10/13 19:05:21, newt wrote: > This comment isn't correct Done. https://codereview.chromium.org/1393073002/diff/220001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer_android.cc (right): https://codereview.chromium.org/1393073002/diff/220001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.cc:24: DCHECK(j_external_data_use_observer_.is_null()); On 2015/10/13 19:05:21, newt wrote: > This DCHECK is superfluous. Of course the member variable hasn't been > initialized yet -- we're still at the beginning of the constructor. Done. https://codereview.chromium.org/1393073002/diff/220001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer_android.cc:43: for (size_t i = 0; i < url_patterns_.size(); ++i) { On 2015/10/13 19:46:56, sclittle wrote: > nit: If you change |url_patterns_| to a ScopedVector, you can just use a > range-based for loop here. Done.
LGTM https://codereview.chromium.org/1393073002/diff/240001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer.cc (right): https://codereview.chromium.org/1393073002/diff/240001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer.cc:41: for (re2::RE2* pattern : url_patterns_) { nit: can you iterate over "const re2::RE2* pattern : url_patterns_"?
Addressed sclittle comment. newt: I would definitely add you as the reviewer for the downstream CL. It is easier if CLs have same reviewers since the context is quite similar. https://codereview.chromium.org/1393073002/diff/240001/chrome/browser/android... File chrome/browser/android/datausage/external_data_use_observer.cc (right): https://codereview.chromium.org/1393073002/diff/240001/chrome/browser/android... chrome/browser/android/datausage/external_data_use_observer.cc:41: for (re2::RE2* pattern : url_patterns_) { On 2015/10/13 21:59:29, sclittle wrote: > nit: can you iterate over "const re2::RE2* pattern : url_patterns_"? Done.
https://codereview.chromium.org/1393073002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java (right): https://codereview.chromium.org/1393073002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java:20: private static ExternalDataUseObserver create(Context context, long nativePtr) { It's not clear to me why you need the native pointer in this method or in ChromeApplication.createExternalDataUseObserver(). Remove it?
https://codereview.chromium.org/1393073002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java (right): https://codereview.chromium.org/1393073002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java:20: private static ExternalDataUseObserver create(Context context, long nativePtr) { On 2015/10/13 22:38:29, newt wrote: > It's not clear to me why you need the native pointer in this method or in > ChromeApplication.createExternalDataUseObserver(). Remove it? It is not needed right now but this will be passed to downstream constructor and that will use this pointer to call functions on specific native object (immediate use case is to specify the set of regular expressions to use). For example, this would be very similar to how NotifyExternalEstimateProviderAndroidUpdate method is called by downstream code on a specific native object.
https://codereview.chromium.org/1393073002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java (right): https://codereview.chromium.org/1393073002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java:20: private static ExternalDataUseObserver create(Context context, long nativePtr) { On 2015/10/13 23:19:21, tbansal1 wrote: > On 2015/10/13 22:38:29, newt wrote: > > It's not clear to me why you need the native pointer in this method or in > > ChromeApplication.createExternalDataUseObserver(). Remove it? > > It is not needed right now but this will be passed > to downstream constructor and that > will use this pointer to call functions on specific native object > (immediate use case is to specify the set of regular expressions > to use). > > For example, this would be very similar to how > NotifyExternalEstimateProviderAndroidUpdate method is called > by downstream code on a specific native object. I see. I'd strongly recommend against passing around native pointers in Java: it's prone to use-after-free bugs since there's no way to know whether the native pointer is still valid. (similar to passing around raw void pointers in C++) A better pattern is to keep the pointer in exactly one place: in the Java side of the C++ object, and to have the C++ side clear the pointer when it's destroyed. E.g. class ExternalDataUseObserver { private long mNativeExternalDataUseObserver; public ExternalDataUseObserver(long nativeExternalDataUseObserver) { mNativeExternalDataUseObserver = nativeExternalDataUseObserver; } public void doSomethingFancy() { if (mNativeExternalDataUseObserver == 0) return or assert; nativeDoSomethingFancy(mNativeExternalDataUseObserver); } @CalledByNative private void onDestroy() { mNativeExternalDataUseObserver = 0; } private void nativeDoSomethingFancy(long nativeExternalDataUseObserver); } class SubclassOfExternalDataUseObserver extends ExternalDataUseObserver { public void foo() { ... doSomethingFancy(); ... } } and the C++ object's destructor should call Java_ExternalDataUseObserver_onDestroy()
newt: PTAL. Thanks! https://codereview.chromium.org/1393073002/diff/260001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java (right): https://codereview.chromium.org/1393073002/diff/260001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java:20: private static ExternalDataUseObserver create(Context context, long nativePtr) { On 2015/10/13 23:43:09, newt wrote: > On 2015/10/13 23:19:21, tbansal1 wrote: > > On 2015/10/13 22:38:29, newt wrote: > > > It's not clear to me why you need the native pointer in this method or in > > > ChromeApplication.createExternalDataUseObserver(). Remove it? > > > > It is not needed right now but this will be passed > > to downstream constructor and that > > will use this pointer to call functions on specific native object > > (immediate use case is to specify the set of regular expressions > > to use). > > > > For example, this would be very similar to how > > NotifyExternalEstimateProviderAndroidUpdate method is called > > by downstream code on a specific native object. > > I see. I'd strongly recommend against passing around native pointers in Java: > it's prone to use-after-free bugs since there's no way to know whether the > native pointer is still valid. (similar to passing around raw void pointers in > C++) > > A better pattern is to keep the pointer in exactly one place: in the Java side > of the C++ object, and to have the C++ side clear the pointer when it's > destroyed. E.g. > > class ExternalDataUseObserver { > private long mNativeExternalDataUseObserver; > > public ExternalDataUseObserver(long nativeExternalDataUseObserver) { > mNativeExternalDataUseObserver = nativeExternalDataUseObserver; > } > > public void doSomethingFancy() { > if (mNativeExternalDataUseObserver == 0) return or assert; > nativeDoSomethingFancy(mNativeExternalDataUseObserver); > } > > @CalledByNative > private void onDestroy() { > mNativeExternalDataUseObserver = 0; > } > > private void nativeDoSomethingFancy(long nativeExternalDataUseObserver); > } > > class SubclassOfExternalDataUseObserver extends ExternalDataUseObserver { > public void foo() { > ... > doSomethingFancy(); > ... > } > } > > and the C++ object's destructor should call > Java_ExternalDataUseObserver_onDestroy() Thanks for the detailed comment. I have fixed the pattern. (Probably tomorrow), I will also send the upstream and downstream CLs to fix the pattern in the other class (ExternalEstimateProvider).
Thanks for addressing. lgtm after one nit. https://codereview.chromium.org/1393073002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java (right): https://codereview.chromium.org/1393073002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java:29: /* nit: use two asterisks "**"
https://codereview.chromium.org/1393073002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java (right): https://codereview.chromium.org/1393073002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/ExternalDataUseObserver.java:29: /* On 2015/10/14 00:02:48, newt wrote: > nit: use two asterisks "**" Done.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, sclittle@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1393073002/#ps320001 (title: "assert added")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1393073002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1393073002/320001
Message was sent while issue was closed.
Committed patchset #8 (id:320001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/796988c4f597a0942455522351e5f2703ccea95d Cr-Commit-Position: refs/heads/master@{#353950} |
