Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1757163002:
[ImportantSites] JNI bindings for CBD filtering and Important Sites. (Closed)
Description[ImportantSites] JNI bindings for CBD filtering and Important Sites.
This CL adds the JNI hookup between the Android java source and the
pref_service_bridge for:
1: deleting the browsing data while excluding a list of domains, and
2: fetching the list of important registerable domains.
See https://goto.google.com/importantsites for more details.
BUG=579763
Committed: https://crrev.com/d1cdbb23d12be8c4388cdc32365130c8238cd118
Cr-Commit-Position: refs/heads/master@{#389267}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Added ImportantSitesUtil, cleaned up #
Total comments: 16
Patch Set 3 : comments #Patch Set 4 : rebase #
Total comments: 7
Patch Set 5 : comments #
Total comments: 16
Patch Set 6 : comments #
Total comments: 1
Patch Set 7 : nit #Messages
Total messages: 34 (11 generated)
dmurph@chromium.org changed reviewers: + msramek@chromium.org, newt@chromium.org, ttr314@googlemail.com
Hello all, This is the follow up patch for https://codereview.chromium.org/1741123002 which would hook up the new origin filtering to the Android side. I have the actual hookup bit commented out. Please let me know if this looks like the correct approach. Thanks, Daniel
Hi Dan! Routing the call through PrefService<->pref_service_bridge is indeed the correct approach. However, please avoid the duplication here. Implement the call without origin list by passing excluding_origins as an empty array or nullptr. Implement ClearBrowsingDataObserver by explicitly passing PrefServiceBridge#browsingDataCleared ClearBrowsingDataObserverWithCallback. pref_service_bridge functionality is difficult to test (and in fact, it's pretty much untested), and I have burned myself on this very recently. This duplication just invites bugs of changing one method and forgetting the other.
https://codereview.chromium.org/1757163002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1757163002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:694: public void clearBrowsingDataExcludingOrigins( javadoc for all public methods Also, how will this method be used? In particular, where will the list of origins come from? IIUC, the intended use for this is to allow clearing data from all sites except "important" sites. If that's the case, would it be simpler or better just to have a method called "clearBrowsingDataExcludingImportantOrigins()", which doesn't take a list of origins but rather builds that list on the C++ side? https://codereview.chromium.org/1757163002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:695: OnClearBrowsingDataListener listener, int[] dataTypes, String[] origins) { Side note: Could you rename this listener to "ClearBrowsingDataListener"? "OnClearBrowsingDataListener" is not idiomatic naming. https://codereview.chromium.org/1757163002/diff/1/chrome/browser/android/pref... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1757163002/diff/1/chrome/browser/android/pref... chrome/browser/android/preferences/pref_service_bridge.cc:503: class ClearBrowsingDataObserverWithCallback There's no reason to add this new class; just modify ClearBrowsingDataObserver above (which is currently unused in this patch set). Also, please remove mClearBrowsingDataListener and browsingDataCleared() from PrefServiceBridge, since they won't be used after this patch. https://codereview.chromium.org/1757163002/diff/1/chrome/browser/android/pref... chrome/browser/android/preferences/pref_service_bridge.cc:519: if (weak_java_callback_.get(env).is_null()) Incidentally, this use of the weak pointer is broken and prone to NPEs. If garbage collection runs after line 519, but before line 522, then we'll get a null pointer exception when calling "weak_java_callback_.get(env).obj()" on line 523. As explained here https://www.ibm.com/support/knowledgecenter/SSYKE2_8.0.0/com.ibm.java.lnx.80.... : "It is not safe to call most JNI functions with a weak global reference, even if you have tested that the reference is not null, because the weak global reference could become a null reference after it has been tested or even during the JNI function. Instead, a weak global reference should always be promoted to a strong reference before it is used. You can promote a weak global reference using the NewLocalRef or NewGlobalRef JNI functions." Anyway, avoid weak references when possible for reasons mentioned below. https://codereview.chromium.org/1757163002/diff/1/chrome/browser/android/pref... chrome/browser/android/preferences/pref_service_bridge.cc:527: JavaObjectWeakGlobalRef weak_java_callback_; This shouldn't be weak; it should just be a regular ScopedJavaGlobalRef. Use weak pointers sparingly, since they can introduce nondeterministic behavior that depends on the whims of the garbage collector. For example, imagine this call PrefServiceBridge.clearBrowsingDataExcludingOrigins(new ClearBrowsingDataObserver() { @Override public void onBrowsingDataCleared() { hideProgressDialog(); } }, ...); What happens if the garbage collector runs before the browsing data has been cleared? In that case, the otherwise unreferenced ClearBrowsingDataObserver would be gc'd and would never receive a callback and the progress dialog would never be dismissed. This is definitely not what we want. A normal non-weak reference should be used here. https://codereview.chromium.org/1757163002/diff/1/chrome/browser/android/pref... chrome/browser/android/preferences/pref_service_bridge.cc:632: static void ClearBrowsingDataExcludingOrigins( As Martin mentioned, please combine this method with the method above. If we're clearing all browsing data (and excluding any origins), you could pass "null" instead of an array of excluded origins. https://codereview.chromium.org/1757163002/diff/1/chrome/browser/android/pref... chrome/browser/android/preferences/pref_service_bridge.cc:637: const JavaParamRef<jobjectArray>& excluding_origins) { call this "excluded_origins" or just "origins"
Thanks so much for the info. I'll combine the two methods. Patch coming soon.
Description was changed from ========== Android CBD origin filter JNI part. This CL adds the JNI hookup between the Android java source and the pref_service_bridge for deleting the browsing data while excluding a list of origins. BUG=579763 ========== to ========== Android CBD origin filter JNI part. This CL adds the JNI hookup between the Android java source and the pref_service_bridge for: 1: deleting the browsing data while excluding a list of domains, and 2: fetching the list of important registerable domains. See https://goto.google.com/importantsites for more details. BUG=579763 ==========
dmurph@chromium.org changed reviewers: + calamity@chromium.org, dominickn@chromium.org
Hello all, This CL has change a little. First, I added the ImportantSitesUtil, which Dominick was helping review earlier. I need this logic hooked up to JNI to access from android preferences, specifically in the CBD screen, Storage site data, and a new activity. Second, I cleanup the JNI code like asked, and added the second JNI call for the ImportantSitesUtil. This is designed to accommodate an async operation (which it may be in the future), but as you can see it's currently actually synchronous. Thanks! Daniel
Description was changed from ========== Android CBD origin filter JNI part. This CL adds the JNI hookup between the Android java source and the pref_service_bridge for: 1: deleting the browsing data while excluding a list of domains, and 2: fetching the list of important registerable domains. See https://goto.google.com/importantsites for more details. BUG=579763 ========== to ========== [ImportantSites] JNI bindings for CBD filtering and Important Sites. This CL adds the JNI hookup between the Android java source and the pref_service_bridge for: 1: deleting the browsing data while excluding a list of domains, and 2: fetching the list of important registerable domains. See https://goto.google.com/importantsites for more details. BUG=579763 ==========
site engagement and its usage lgtm Query: Do you have any tie-breaking plans other than the order in which the content settings are retrieved? That being said, we're going to try and make sure that multiple sites won't bunch up at 100 engagement (by slowing engagement earning to a crawl once it hits 99/100) - that will hopefully be in M52.
dmurph@chromium.org changed reviewers: + twellington@chromium.org - newt@chromium.org
-newt, +twellington Hi Theresa! Ted said you were taking over newt's reviews here. I have some settings changes (here and https://codereview.chromium.org/1465363002) for adding important sites to Chrome. This patch is the JNI hookups I need to C++ to fetch important sites, and then optionally filter out important sites when I clear browsing data. See go/importantsites for more info & mocks. Let me know if you have any questions! Daniel
https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:88: public abstract void setImportantRegisterableDomains(String[] domains); This public method needs a JavaDoc. https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:748: * DO NOT USE THIS METHOD UNLESS CALLER KNOWS WHAT THEY'RE DOING. NOT ALL BACKENDS ARE nit: I'd prefer this wasn't in all caps. https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:773: public void fetchImportantSites(ImportantSitesCallback callback) { This needs a JavaDoc too https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:774: assert mImportantSitesCallback == null; Is there a case where this may get called multiple times? I'm wondering if it may be better just to return if mImportantSitesCallback != null rather than assert null. https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:775: mImportantSitesCallback = callback; Since this method just sets the important sets callback would it be more appropriately named "setImportantSitesCallback"? https://codereview.chromium.org/1757163002/diff/20001/chrome/browser/android/... File chrome/browser/android/preferences/important_sites_util_unittest.cc (right): https://codereview.chromium.org/1757163002/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/important_sites_util_unittest.cc:116: // /Same as above, but the site with notifications should be at the front. nit: remove "/" before Same https://codereview.chromium.org/1757163002/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/important_sites_util_unittest.cc:118: "chrome.com", "google.com"}; Is CONTENT_SETTING_BLOCK not a notification that moves things to the front of this sorted list?
Thanks for the review! Let me know what you think about the PrefServiceBridge style for the callback. I made it like the old clearBrowsingData callback, but it's not exactly pretty. We do things slightly different in WebsiteSettingsPreferenceBridge, but apparently that uses slightly bad C++-side JNI style? https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:88: public abstract void setImportantRegisterableDomains(String[] domains); On 2016/04/20 at 00:45:30, Theresa Wellington wrote: > This public method needs a JavaDoc. Done. https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:748: * DO NOT USE THIS METHOD UNLESS CALLER KNOWS WHAT THEY'RE DOING. NOT ALL BACKENDS ARE On 2016/04/20 at 00:45:30, Theresa Wellington wrote: > nit: I'd prefer this wasn't in all caps. Done. https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:773: public void fetchImportantSites(ImportantSitesCallback callback) { On 2016/04/20 at 00:45:30, Theresa Wellington wrote: > This needs a JavaDoc too Done. https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:774: assert mImportantSitesCallback == null; On 2016/04/20 at 00:45:30, Theresa Wellington wrote: > Is there a case where this may get called multiple times? I'm wondering if it may be better just to return if mImportantSitesCallback != null rather than assert null. Well, I think we're trying to protect against accidentally having this called multiple times on the same object before the callback is triggered. Protecting against programmer mistake, which I think we should prefer failing for. Let me know if you feel strongly though, I can switch it to anything. https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:775: mImportantSitesCallback = callback; On 2016/04/20 at 00:45:30, Theresa Wellington wrote: > Since this method just sets the important sets callback would it be more appropriately named "setImportantSitesCallback"? Whoops, I meant to call the native method. I'm modelling this off of "clearBrowsingData" above, which does it like this. Let me know if you want me to do it a different way, I fixed it to match the above method. https://codereview.chromium.org/1757163002/diff/20001/chrome/browser/android/... File chrome/browser/android/preferences/important_sites_util_unittest.cc (right): https://codereview.chromium.org/1757163002/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/important_sites_util_unittest.cc:116: // /Same as above, but the site with notifications should be at the front. On 2016/04/20 at 00:45:30, Theresa Wellington wrote: > nit: remove "/" before Same Done. https://codereview.chromium.org/1757163002/diff/20001/chrome/browser/android/... chrome/browser/android/preferences/important_sites_util_unittest.cc:118: "chrome.com", "google.com"}; On 2016/04/20 at 00:45:30, Theresa Wellington wrote: > Is CONTENT_SETTING_BLOCK not a notification that moves things to the front of this sorted list? Done.
lgtm https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:774: assert mImportantSitesCallback == null; On 2016/04/20 22:06:28, dmurph wrote: > On 2016/04/20 at 00:45:30, Theresa Wellington wrote: > > Is there a case where this may get called multiple times? I'm wondering if it > may be better just to return if mImportantSitesCallback != null rather than > assert null. > > Well, I think we're trying to protect against accidentally having this called > multiple times on the same object before the callback is triggered. Protecting > against programmer mistake, which I think we should prefer failing for. > > Let me know if you feel strongly though, I can switch it to anything. Just to confirm, there should only ever be one active caller for this method (e.g. two classes won't both be requesting important site info)? If that's the case, an assert is fine with me. https://codereview.chromium.org/1757163002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:775: mImportantSitesCallback = callback; On 2016/04/20 22:06:28, dmurph wrote: > On 2016/04/20 at 00:45:30, Theresa Wellington wrote: > > Since this method just sets the important sets callback would it be more > appropriately named "setImportantSitesCallback"? > > Whoops, I meant to call the native method. I'm modelling this off of > "clearBrowsingData" above, which does it like this. Let me know if you want me > to do it a different way, I fixed it to match the above method. This looks good. https://codereview.chromium.org/1757163002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1757163002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:89: * Called by native code to populate the set of important registerable domains. Technically this isn't called by native code (it's called in a Java method below). Maybe something like "Called when the list of important registerable domains has been fetched."? Nit on the @param line -- capitalize Important. https://codereview.chromium.org/1757163002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:757: public void clearBrowsingDataExcludingDomains(OnClearBrowsingDataListener listener, add JavaDocs for the params (or at least the extra origins param) please -- also consider renaming to blacklistDomains to match the native method https://codereview.chromium.org/1757163002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:786: * @param callback the callback that will be used to set the list of important sites. nit: capitalize The
Thanks for the review! Any suggestion for someone I should grab for owners approval? https://codereview.chromium.org/1757163002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1757163002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:89: * Called by native code to populate the set of important registerable domains. On 2016/04/21 at 16:05:28, Theresa Wellington wrote: > Technically this isn't called by native code (it's called in a Java method below). Maybe something like "Called when the list of important registerable domains has been fetched."? > > Nit on the @param line -- capitalize Important. Done. https://codereview.chromium.org/1757163002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:757: public void clearBrowsingDataExcludingDomains(OnClearBrowsingDataListener listener, On 2016/04/21 at 16:05:28, Theresa Wellington wrote: > add JavaDocs for the params (or at least the extra origins param) please -- also consider renaming to blacklistDomains to match the native method Done. https://codereview.chromium.org/1757163002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:757: public void clearBrowsingDataExcludingDomains(OnClearBrowsingDataListener listener, On 2016/04/21 at 16:05:28, Theresa Wellington wrote: > add JavaDocs for the params (or at least the extra origins param) please -- also consider renaming to blacklistDomains to match the native method Done. https://codereview.chromium.org/1757163002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:786: * @param callback the callback that will be used to set the list of important sites. On 2016/04/21 at 16:05:28, Theresa Wellington wrote: > nit: capitalize The Done.
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1757163002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1757163002/80001
On 2016/04/21 18:53:25, dmurph wrote: > Thanks for the review! > Any suggestion for someone I should grab for owners approval? Just chatted with tedchoc@ and he said to add him for owners approval.
dmurph@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc for approval. Thanks! Daniel
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1757163002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1757163002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:87: public interface ImportantSitesCallback { You might want to consider just using a plan base/Callback<String[]> here instead of defining a new class type. Or maybe you don't need a callback at all per my comment in the C++ side of things. https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/important_sites_util.cc (right): https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/important_sites_util.cc:54: if (!origin.is_valid() || site.setting != CONTENT_SETTING_ALLOW) I would check the setting before the GURL conversion since that is slightly more complicated. https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:607: std::vector<std::string> cpp_excluding_domains; I would say the common naming scheme would be to call the java param jexcluding_domains then this would be just excluding_domains https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:629: ImportantSitesUtil::GetImportantRegisterableDomains(GetOriginalProfile(), is this guaranteed to be fast? looks "scary-ish" to be done on the UI thread, but maybe it is all in memory/quick/etc... https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:633: Java_PrefServiceBridge_importantSitesFetched(env, obj.obj(), Looking at the java side, I expected this to be an async call since you are tracking a callback. why not just return the string list instead of taking a callback? It is a bit clearer from an API standpoint and you wouldn't get unexpected timing issues w/ the immediate callback triggered here.
Hi, some quick replies for a couple comments. Let me know if you have strong opinions here, I'll also add comments to the code clarifying what's happening and why we chose the async model. Daniel https://codereview.chromium.org/1757163002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1757163002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:87: public interface ImportantSitesCallback { On 2016/04/21 at 20:18:16, Ted C wrote: > You might want to consider just using a plan base/Callback<String[]> here instead of defining a new class type. > > Or maybe you don't need a callback at all per my comment in the C++ side of things. That could work too. I was just modelling the current callback method above to stay consistant. Let me know if you have strong feelings here. https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:629: ImportantSitesUtil::GetImportantRegisterableDomains(GetOriginalProfile(), On 2016/04/21 at 20:18:16, Ted C wrote: > is this guaranteed to be fast? looks "scary-ish" to be done on the UI thread, but maybe it is all in memory/quick/etc... It's all in memory. If we ever need to hit disk (which we may need to soon), then I'll be putting this on a different thread. But we have the callback set up to handle this asynchronous behavior. https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:633: Java_PrefServiceBridge_importantSitesFetched(env, obj.obj(), On 2016/04/21 at 20:18:16, Ted C wrote: > Looking at the java side, I expected this to be an async call since you are tracking a callback. why not just return the string list instead of taking a callback? > > It is a bit clearer from an API standpoint and you wouldn't get unexpected timing issues w/ the immediate callback triggered here. I expect this call to not always be synchronous, so I'd rather make the API support that right away.
https://codereview.chromium.org/1757163002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1757163002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:87: public interface ImportantSitesCallback { On 2016/04/21 20:31:23, dmurph wrote: > On 2016/04/21 at 20:18:16, Ted C wrote: > > You might want to consider just using a plan base/Callback<String[]> here > instead of defining a new class type. > > > > Or maybe you don't need a callback at all per my comment in the C++ side of > things. > > That could work too. I was just modelling the current callback method above to > stay consistant. Let me know if you have strong feelings here. For callbacks I have a slight preference towards not creating these one off classes (the one above is a listener), but I'm not convinced it needed to be anything other than a Runnable there either. But I don't feel too strongly, so do as you wish here. https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:633: Java_PrefServiceBridge_importantSitesFetched(env, obj.obj(), On 2016/04/21 20:31:23, dmurph wrote: > On 2016/04/21 at 20:18:16, Ted C wrote: > > Looking at the java side, I expected this to be an async call since you are > tracking a callback. why not just return the string list instead of taking a > callback? > > > > It is a bit clearer from an API standpoint and you wouldn't get unexpected > timing issues w/ the immediate callback triggered here. > > I expect this call to not always be synchronous, so I'd rather make the API > support that right away. Then a couple things. 1.) Pass down the java callback to native and track it along with the request. Having it a one at a time thing in java land is gross and following clear browsing isn't what I would do. 2.) Post the callback in the javaside for now to ensure it is infact async because otherwise the callers might not be happy with what happens.
Thanks for the comments, made it resemble WebPreferenceBridge a bit. https://codereview.chromium.org/1757163002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1757163002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:87: public interface ImportantSitesCallback { On 2016/04/21 at 21:23:39, Ted C wrote: > On 2016/04/21 20:31:23, dmurph wrote: > > On 2016/04/21 at 20:18:16, Ted C wrote: > > > You might want to consider just using a plan base/Callback<String[]> here > > instead of defining a new class type. > > > > > > Or maybe you don't need a callback at all per my comment in the C++ side of > > things. > > > > That could work too. I was just modelling the current callback method above to > > stay consistant. Let me know if you have strong feelings here. > > For callbacks I have a slight preference towards not creating these one off classes (the one above is a listener), but I'm not convinced it needed to be anything other than a Runnable there either. > > But I don't feel too strongly, so do as you wish here. Due to passing the callback directly to CPP, I'm going to have the keep the interface (I think) to annotate it as @CalledByNative. Is that correct? https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/important_sites_util.cc (right): https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/important_sites_util.cc:54: if (!origin.is_valid() || site.setting != CONTENT_SETTING_ALLOW) On 2016/04/21 at 20:18:16, Ted C wrote: > I would check the setting before the GURL conversion since that is slightly more complicated. Done. https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:607: std::vector<std::string> cpp_excluding_domains; On 2016/04/21 at 20:18:16, Ted C wrote: > I would say the common naming scheme would be to call the java param jexcluding_domains then this would be just excluding_domains Done. https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:633: Java_PrefServiceBridge_importantSitesFetched(env, obj.obj(), On 2016/04/21 at 21:23:39, Ted C wrote: > On 2016/04/21 20:31:23, dmurph wrote: > > On 2016/04/21 at 20:18:16, Ted C wrote: > > > Looking at the java side, I expected this to be an async call since you are > > tracking a callback. why not just return the string list instead of taking a > > callback? > > > > > > It is a bit clearer from an API standpoint and you wouldn't get unexpected > > timing issues w/ the immediate callback triggered here. > > > > I expect this call to not always be synchronous, so I'd rather make the API > > support that right away. > > Then a couple things. > > 1.) Pass down the java callback to native and track it along with the request. Having it a one at a time thing in java land is gross and following clear browsing isn't what I would do. > > 2.) Post the callback in the javaside for now to ensure it is infact async because otherwise the callers might not be happy with what happens. How does this look? I pass the callback down.
lgtm thanks! https://codereview.chromium.org/1757163002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1757163002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:87: public interface ImportantSitesCallback { On 2016/04/22 18:06:22, dmurph wrote: > On 2016/04/21 at 21:23:39, Ted C wrote: > > On 2016/04/21 20:31:23, dmurph wrote: > > > On 2016/04/21 at 20:18:16, Ted C wrote: > > > > You might want to consider just using a plan base/Callback<String[]> here > > > instead of defining a new class type. > > > > > > > > Or maybe you don't need a callback at all per my comment in the C++ side > of > > > things. > > > > > > That could work too. I was just modelling the current callback method above > to > > > stay consistant. Let me know if you have strong feelings here. > > > > For callbacks I have a slight preference towards not creating these one off > classes (the one above is a listener), but I'm not convinced it needed to be > anything other than a Runnable there either. > > > > But I don't feel too strongly, so do as you wish here. > > Due to passing the callback directly to CPP, I'm going to have the keep the > interface (I think) to annotate it as @CalledByNative. Is that correct? Indeed! https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1757163002/diff/80001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:633: Java_PrefServiceBridge_importantSitesFetched(env, obj.obj(), On 2016/04/22 18:06:22, dmurph wrote: > On 2016/04/21 at 21:23:39, Ted C wrote: > > On 2016/04/21 20:31:23, dmurph wrote: > > > On 2016/04/21 at 20:18:16, Ted C wrote: > > > > Looking at the java side, I expected this to be an async call since you > are > > > tracking a callback. why not just return the string list instead of taking > a > > > callback? > > > > > > > > It is a bit clearer from an API standpoint and you wouldn't get unexpected > > > timing issues w/ the immediate callback triggered here. > > > > > > I expect this call to not always be synchronous, so I'd rather make the API > > > support that right away. > > > > Then a couple things. > > > > 1.) Pass down the java callback to native and track it along with the request. > Having it a one at a time thing in java land is gross and following clear > browsing isn't what I would do. > > > > 2.) Post the callback in the javaside for now to ensure it is infact async > because otherwise the callers might not be happy with what happens. > > How does this look? I pass the callback down. Perfect https://codereview.chromium.org/1757163002/diff/100001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1757163002/diff/100001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:93: public abstract void onImportantRegisterableDomainsReady(String[] domains); nit, drop public and abstract. Both are implied and default for interfaces and we shouldn't ever need them
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, twellington@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/1757163002/#ps120001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1757163002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1757163002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [ImportantSites] JNI bindings for CBD filtering and Important Sites. This CL adds the JNI hookup between the Android java source and the pref_service_bridge for: 1: deleting the browsing data while excluding a list of domains, and 2: fetching the list of important registerable domains. See https://goto.google.com/importantsites for more details. BUG=579763 ========== to ========== [ImportantSites] JNI bindings for CBD filtering and Important Sites. This CL adds the JNI hookup between the Android java source and the pref_service_bridge for: 1: deleting the browsing data while excluding a list of domains, and 2: fetching the list of important registerable domains. See https://goto.google.com/importantsites for more details. BUG=579763 Committed: https://crrev.com/d1cdbb23d12be8c4388cdc32365130c8238cd118 Cr-Commit-Position: refs/heads/master@{#389267} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/d1cdbb23d12be8c4388cdc32365130c8238cd118 Cr-Commit-Position: refs/heads/master@{#389267} |
