Miscellaneous fixes to DataUse UI

chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java

Index: chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java b/chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java
index c353b1fbfe7cc347506f2d2258713b2197a2bd40..4b89a0bf2e0153e6567e23134475d82fbb4d38be 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java
@@ -60,6 +60,33 @@ public class DataUseTabUIManager {
     }
 
     /**
+     * Notifies that the user clicked "Continue" when the dialog box warning about exiting data use
+     * was shown.
+     *
+     * @param tab The tab on which the dialog box was shown.
+     */
+    public static void userClickedContinueOnDialogBox(Tab tab) {
+        nativeUserClickedContinueOnDialogBox(
+                SessionTabHelper.sessionIdForTab(tab.getWebContents()), tab.getProfile());
+    }
+
+    /**
+     * Returns true if data use tracking is currently active on {@link tab} but will stop if the
+     * navigation continues.
+     *
+     * @param tab The tab that is being queried for data use tracking.
+     * @param pageTransitionType transition type of the navigation
+     * @param packageName package name of the app package that started this navigation.
+     * @return true If {@link tab} is currently tracked but would stop if the navigation were to
+     * continue.
+     */
+    public static boolean wouldDataUseTrackingEnd(

[megjablon, 2015/12/18 20:17:53]

Don't we still want to remove the event even if the user cancels? Won't there be
a new tracking ended event when the user tries to navigate away again?

[tbansal1, 2015/12/18 21:35:37]

I am not sure if I understood the comment. But, there are two cases:
(i) User clicks "Continue". In that case, |tab_id| will be set to
DATA_USE_CONTINUE_CLICKED. Java will call WouldDataUseTrackingEnd again when it
restarts navigation. That function would see that |tab_id| is set to
DATA_USE_CONTINUE_CLICKED, and it will return false.
(ii) User clicks "Cancel". In that case nothing changes: The tab would still be
tracked. If the user tries to navigate away, it will repeat the whole process
aagian (WouldDataUseTrackingEnd() would be called, and dialog may be shown).

[megjablon, 2015/12/18 22:08:10]

What I don't understand is why we need both wouldDataUseTrackingEnd and
checkDataUseTrackingEnded and why we can't look for the tracking ended event. Is
this to fix the timing issue?

[tbansal1, 2015/12/18 22:33:09]

wouldDataUseTrackingEnd is called before the navigation starts (as part of
throttling) checks if the navigation would end data use tracking. If it returns
true, dialog box is shown. It does not change the state of the tab or any other
tracking records that are maintained. We can't look for tracking ended event
because tracking does not end until user clicks "Continue", and navigation goes
through. At that point NotifyTackingEnd() would be called, which would tell us
that tracking has ended.

checkDataUseTrackingEnded is called after the navigation is done. It is used for
showing snackbar first time the tracking ends. This checks if the tracking has
already ended, and some UI needs to be shown at that point. If a UI needs to be
shown, it remembers that. So, if user does another navigation, 
checkDataUseTrackingEnded will be called again, but it will return false. 

[megjablon, 2015/12/28 20:32:06]

Why can't we just use wouldDataUseTrackingEnd in place of
checkDataUseTrackingEnded for the snackbar and remove checkDataUseTrackingEnded
altogether? Then we wouldn't have to store the ended state. If the snackbar
knows tracking is going to end, then it can just show the snackbar.

[tbansal1, 2015/12/28 23:55:52]

Timeline for some tab with id t:
1) Some navigation happens on t
2) t is now tracked
3) Some navigation is initiated. wouldDataUseTrackingEnd is called to check if
this navigation would
stop tracking data use
4) navigation is started (may be because user clicked Continue)
5) checkDataUseTrackingEnded is called to see if a UI snackbar needs to be
shown. It may need to be shown if the pref is set to not show the dialog box.


wouldDataUseTrackingEnd returns true if tab is currently being tracked but would
not be tracked if the navigation were to happen.

OTOH, checkDataUseTrackingEnded returns true if tab is not being tracked and a
snackbar is pending to be shown to the user.

By the time, checkDataUseTrackingEnded is called, navigation has started, and
tracking has already ended. So, if we replace checkDataUseTrackingEnded by
wouldDataUseTrackingEnd in step 5 above, then it would always return false.

[megjablon, 2015/12/29 00:05:21]

Makes sense. I wasn't thinking about the fact that we moved back the timing of
the snackbar. I thought they were both being called too early. Would this still
be true if the snackbar did it's check in onPageLoadStarted?

[tbansal1, 2015/12/29 00:09:35]

We would still need different functions even if the check was in
onPageLoadStarted.

You are right. The two checks are not called at the same place.
wouldDataUseTrackingEnd is called before 
navigation starts. At that point, there is an option of throttling the
navigation if the user clicks "Cancels". After that navigation may happen (if
the user clicks "Continue"), and then  onPageLoadStarted() and
didFirstVisuallyNonEmptyPaint() are called. So, wouldDataUseTrackingEnd and
checkDataUseTrackingEnded  are not called at the same time, and they serve
different purposes.

[megjablon, 2015/12/29 00:38:24]

Ok cool, SGTM!

+            Tab tab, String url, int pageTransitionType, String packageName) {
+        return nativeWouldDataUseTrackingEnd(SessionTabHelper.sessionIdForTab(tab.getWebContents()),
+                url, pageTransitionType, packageName, tab.getProfile());
+    }
+
+    /**
      * Returns true if data use tracking has ended within a Tab. When data use tracking has
      * ended, returns true only once to signify the ended event.
      *
@@ -99,7 +126,8 @@ public class DataUseTabUIManager {
     public static boolean shouldOverrideUrlLoading(Activity activity,
             final Tab tab, final String url, final int pageTransitionType,
             final String referrerUrl) {
-        if (!getOptedOutOfDataUseDialog(activity) && checkDataUseTrackingEnded(tab)) {
+        if (!getOptedOutOfDataUseDialog(activity)
+                && wouldDataUseTrackingEnd(tab, url, pageTransitionType, "")) {
             startDataUseDialog(activity, tab, url, pageTransitionType, referrerUrl);
             return true;
         }
@@ -147,6 +175,7 @@ public class DataUseTabUIManager {
                                 }
                                 tab.loadUrl(loadUrlParams);
                                 recordDataUseUIAction(DataUseUIActions.DIALOG_CONTINUE_CLICKED);
+                                userClickedContinueOnDialogBox(tab);
                             }
                         })
                 .setNegativeButton(R.string.cancel, new OnClickListener() {
@@ -199,6 +228,9 @@ public class DataUseTabUIManager {
 
     private static native boolean nativeCheckDataUseTrackingStarted(int tabId, Profile profile);
     private static native boolean nativeCheckDataUseTrackingEnded(int tabId, Profile profile);
+    private static native void nativeUserClickedContinueOnDialogBox(int tabId, Profile profile);
+    private static native boolean nativeWouldDataUseTrackingEnd(
+            int tabId, String url, int pageTransitionType, String packageName, Profile jprofile);
     private static native void nativeOnCustomTabInitialNavigation(int tabID, String packageName,
             String url, Profile profile);
 }