Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1870703002:
Connect the "other forms of browsing history" frontend on Android to backend. (Closed)
DescriptionConnect the "other forms of browsing history" frontend on Android to backend.
When created, ClearBrowsingDataPreferences asks through
the PrefServiceBridge whether it should display the notice
about other forms of browsing history in the footer and in
the dialog. The response is returned asynchronously.
BUG=595332
Committed: https://crrev.com/cf14b5cb372037e46ffc3bae09cddd2af1ca4b51
Cr-Commit-Position: refs/heads/master@{#386140}
Patch Set 1 : #
Total comments: 16
Patch Set 2 : Rebase. #Patch Set 3 : Addressed comments. #
Total comments: 4
Depends on Patchset: Messages
Total messages: 17 (9 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Connect the "other forms of browsing history" frontend on Android to backend. BUG=595332 ========== to ========== Connect the "other forms of browsing history" frontend on Android to backend. When created, ClearBrowsingDataPreferences asks through the PrefServiceBridge whether it should display the notice about other forms of browsing history in the footer and in the dialog. The response is returned asynchronously. BUG=595332 ==========
Patchset #1 (id:20001) has been deleted
msramek@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dan, can you please have a look at this one as well? It's a follow-up to https://codereview.chromium.org/1859373002/ that I sent you yesterday, hooking the frontend built in that CL to the backend logic. Thanks, Martin
https://chromiumcodereview.appspot.com/1870703002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://chromiumcodereview.appspot.com/1870703002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:737: public interface OtherFormsOfBrowsingHistoryListener { public inner classes go at the top of the class. also add javadocs for the functions. https://chromiumcodereview.appspot.com/1870703002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:743: private OtherFormsOfBrowsingHistoryListener mEnableDialogListener; Same comment as the other CL: don't randomly put member fields in the middle of a class. The class is admittedly already a bit messy, but these should go beneath: private static PrefServiceBridge sInstance; https://chromiumcodereview.appspot.com/1870703002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:752: // Null the listener so that ClearBrowsingDataPreferences can be garbage-collected. This would happen regardless, so the comment isn't useful. https://chromiumcodereview.appspot.com/1870703002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:758: private void enableDialogAboutOtherFormsOfBrowsingHistory(boolean enable) { I don't understand why you grouped those functions together in your interface, then kept two separate references to the same class; it just clutters up the logic. You may as well just make them Runnables: private Runnable mShowNoticeRunnable; private Runnable mEnableDialogRunnable; @CalledByNative private void showNoticeAboutOtherFormsOfBrowsingHistory(boolean show) { if (mShowNoticeRunnable != null && show) mShowNoticeRunnable.run(); mShowNoticeRunnable = null; } @CalledByNative private void enableDialogAboutOtherFormsOfBrowsingHistory(boolean enable) { if (mEnableDialogRunnable != null && enable) mEnableDialogRunnable.run(); mEnableDialogRunnable = null; } https://chromiumcodereview.appspot.com/1870703002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:779: nativeRequestInfoAboutOtherFormsOfBrowsingHistory(); Instead of storing the listeners, you should pass the callback directly to the native code and run the appropriate one when necessary. Would avoid new member fields, and would avoid any weirdness where the function was called twice with two different OtherFormsOfBrowsingHistoryListeners. https://chromiumcodereview.appspot.com/1870703002/diff/40001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://chromiumcodereview.appspot.com/1870703002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:466: * service when it discovers that they exist. nits: 1) Called by the web history etc etc 2) This comment should be on the interface itself. https://chromiumcodereview.appspot.com/1870703002/diff/40001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:481: * after deleting their Chrome history. To be called by the web history service when the Ditto. https://chromiumcodereview.appspot.com/1870703002/diff/40001/chrome/browser/a... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://chromiumcodereview.appspot.com/1870703002/diff/40001/chrome/browser/a... chrome/browser/android/preferences/pref_service_bridge.cc:613: static void showNoticeAboutOtherFormsOfBrowsingHistory( nit: You're in C++ land: ShowNoticeAboutOtherFormsOfBrowsingHistory() EnableDialogAboutOtherFormsOfBrowsingHistory()
Patchset #2 (id:60001) has been deleted
Patchset #3 (id:100001) has been deleted
PTAL! I addressed your comments and it actually made this CL a lot simpler :) https://codereview.chromium.org/1870703002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/1870703002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:737: public interface OtherFormsOfBrowsingHistoryListener { On 2016/04/07 22:27:55, dfalcantara wrote: > public inner classes go at the top of the class. also add javadocs for the > functions. Done. I also moved the existing OnClearBrowsingDataListener up - that is actually what I took as an example when I put this class lower. https://codereview.chromium.org/1870703002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:743: private OtherFormsOfBrowsingHistoryListener mEnableDialogListener; On 2016/04/07 22:27:55, dfalcantara wrote: > Same comment as the other CL: don't randomly put member fields in the middle of > a class. The class is admittedly already a bit messy, but these should go > beneath: > > private static PrefServiceBridge sInstance; Acknowledged. This was removed, as per the other comment. https://codereview.chromium.org/1870703002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:752: // Null the listener so that ClearBrowsingDataPreferences can be garbage-collected. On 2016/04/07 22:27:55, dfalcantara wrote: > This would happen regardless, so the comment isn't useful. Acknowledged. This part is now removed. https://codereview.chromium.org/1870703002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:758: private void enableDialogAboutOtherFormsOfBrowsingHistory(boolean enable) { On 2016/04/07 22:27:55, dfalcantara wrote: > I don't understand why you grouped those functions together in your interface, > then kept two separate references to the same class; it just clutters up the > logic. You may as well just make them Runnables: > > private Runnable mShowNoticeRunnable; > private Runnable mEnableDialogRunnable; > > @CalledByNative > private void showNoticeAboutOtherFormsOfBrowsingHistory(boolean show) { > if (mShowNoticeRunnable != null && show) mShowNoticeRunnable.run(); > mShowNoticeRunnable = null; > } > > @CalledByNative > private void enableDialogAboutOtherFormsOfBrowsingHistory(boolean enable) { > if (mEnableDialogRunnable != null && enable) mEnableDialogRunnable.run(); > mEnableDialogRunnable = null; > } I kept two separate references, because I don't know which callback will return first; I need to keep the reference until both callbacks fire, and then to null it, so that PrefServiceBridge doesn't unnecesarily hold ClearBrowsingDataPreferences. Nevertheless, I'll pass the callback to the native side, as you suggested in the other comment. https://codereview.chromium.org/1870703002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:779: nativeRequestInfoAboutOtherFormsOfBrowsingHistory(); On 2016/04/07 22:27:55, dfalcantara wrote: > Instead of storing the listeners, you should pass the callback directly to the > native code and run the appropriate one when necessary. Would avoid new member > fields, and would avoid any weirdness where the function was called twice with > two different OtherFormsOfBrowsingHistoryListeners. Done. That was actually my first idea, but then I decided to follow the example of browsingDataCleared() which stores the pointer here. But yes, it turns out that implementation is even simpler. https://codereview.chromium.org/1870703002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/1870703002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:466: * service when it discovers that they exist. On 2016/04/07 22:27:55, dfalcantara wrote: > nits: > 1) Called by the web history etc etc > 2) This comment should be on the interface itself. Done. https://codereview.chromium.org/1870703002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:481: * after deleting their Chrome history. To be called by the web history service when the On 2016/04/07 22:27:55, dfalcantara wrote: > Ditto. Done. https://codereview.chromium.org/1870703002/diff/40001/chrome/browser/android/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://codereview.chromium.org/1870703002/diff/40001/chrome/browser/android/... chrome/browser/android/preferences/pref_service_bridge.cc:613: static void showNoticeAboutOtherFormsOfBrowsingHistory( On 2016/04/07 22:27:55, dfalcantara wrote: > nit: You're in C++ land: > > ShowNoticeAboutOtherFormsOfBrowsingHistory() > EnableDialogAboutOtherFormsOfBrowsingHistory() Done.
lgtm Hrm, definitely a lot cleaner. Thanks! https://chromiumcodereview.appspot.com/1870703002/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://chromiumcodereview.appspot.com/1870703002/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:136: private boolean mIsDialogAboutOtherFormsOfBrowsingHistoryEnabled; Clank's Java code is admittedly all over the place with these guidelines, but we generally try to group the top of classes like this to make it easier to find things: public static final constants protected static final constants private static final constants public static classes/interfaces protected static classes/interfaces private static classes/interfaces public final fields protected final fields private final fields public fields protected fields private fields static methods methods https://chromiumcodereview.appspot.com/1870703002/diff/120001/chrome/browser/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://chromiumcodereview.appspot.com/1870703002/diff/120001/chrome/browser/... chrome/browser/android/preferences/pref_service_bridge.cc:629: Java_OtherFormsOfBrowsingHistoryListener_enableDialogAboutOtherFormsOfBrowsingHistory( <random comment about how hilariously long JNI calls are>
Thanks, Dan! https://chromiumcodereview.appspot.com/1870703002/diff/120001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://chromiumcodereview.appspot.com/1870703002/diff/120001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:136: private boolean mIsDialogAboutOtherFormsOfBrowsingHistoryEnabled; On 2016/04/08 17:44:58, dfalcantara wrote: > Clank's Java code is admittedly all over the place with these guidelines, but we > generally try to group the top of classes like this to make it easier to find > things: > > public static final constants > protected static final constants > private static final constants > > public static classes/interfaces > protected static classes/interfaces > private static classes/interfaces > > public final fields > protected final fields > private final fields > > public fields > protected fields > private fields > > static methods > > methods > Acknowledged. These two variables are moved in CL https://chromiumcodereview.appspot.com/1875633003/. The rest of the class still deserves some cleanup, but I'd do that in a later CL. I now also have a sudden realization that the header files in C++ help *a lot* to keep things organized by hiding the function bodies. https://chromiumcodereview.appspot.com/1870703002/diff/120001/chrome/browser/... File chrome/browser/android/preferences/pref_service_bridge.cc (right): https://chromiumcodereview.appspot.com/1870703002/diff/120001/chrome/browser/... chrome/browser/android/preferences/pref_service_bridge.cc:629: Java_OtherFormsOfBrowsingHistoryListener_enableDialogAboutOtherFormsOfBrowsingHistory( On 2016/04/08 17:44:58, dfalcantara wrote: > <random comment about how hilariously long JNI calls are> Yes, I have first searched for "file:.cc Java_.{74}" to see if it's acceptable to go over 80 chars :)
The CQ bit was checked by msramek@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1870703002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1870703002/120001
Message was sent while issue was closed.
Description was changed from ========== Connect the "other forms of browsing history" frontend on Android to backend. When created, ClearBrowsingDataPreferences asks through the PrefServiceBridge whether it should display the notice about other forms of browsing history in the footer and in the dialog. The response is returned asynchronously. BUG=595332 ========== to ========== Connect the "other forms of browsing history" frontend on Android to backend. When created, ClearBrowsingDataPreferences asks through the PrefServiceBridge whether it should display the notice about other forms of browsing history in the footer and in the dialog. The response is returned asynchronously. BUG=595332 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Connect the "other forms of browsing history" frontend on Android to backend. When created, ClearBrowsingDataPreferences asks through the PrefServiceBridge whether it should display the notice about other forms of browsing history in the footer and in the dialog. The response is returned asynchronously. BUG=595332 ========== to ========== Connect the "other forms of browsing history" frontend on Android to backend. When created, ClearBrowsingDataPreferences asks through the PrefServiceBridge whether it should display the notice about other forms of browsing history in the footer and in the dialog. The response is returned asynchronously. BUG=595332 Committed: https://crrev.com/cf14b5cb372037e46ffc3bae09cddd2af1ca4b51 Cr-Commit-Position: refs/heads/master@{#386140} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/cf14b5cb372037e46ffc3bae09cddd2af1ca4b51 Cr-Commit-Position: refs/heads/master@{#386140} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
