Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1539763004:
Miscellaneous fixes to DataUse UI (Closed)
DescriptionMiscellaneous fixes to DataUse UI
- Plumb the notification that user clicked "Continue" when
the dialog box with warning was shown from Java code to
native
- Java code now queries if a navigation would end data use
tracking. This is a simple query which does not change any
state (Previously it used a wrong function which would
change the internal state of the Tab Model)
- Use FrameDeleted instead of RenderFrameDeleted. The former
is the one we want to use
- Move the snackbar to onDidNavigateMainFrame. Currently,
it is called on page load which is too early (underlying
code is not aware of navigation at that point)
- Change the order of |url| and |package_name| in custom tab
notification (it was reversed when the call went from Java
to native).
BUG=570885, 558161, 552133
Committed: https://crrev.com/26f2eb746b39256c3f00eeacc360e7206415555d
Cr-Commit-Position: refs/heads/master@{#367414}
Patch Set 1 : Updated #
Total comments: 18
Patch Set 2 : Addressed megjablon comments #
Total comments: 1
Patch Set 3 : Minor change #Patch Set 4 : Rebased #Patch Set 5 : rebased #Patch Set 6 : Added tests. Finished TODOs #
Total comments: 2
Patch Set 7 : Rebased, addressed megjablon comments #
Total comments: 8
Patch Set 8 : Addressed comments #
Total comments: 2
Patch Set 9 : Addressed newt comment (renamed two functions) #Patch Set 10 : Rebased #Messages
Total messages: 36 (16 generated)
Description was changed from ========== w w w w BUG= ========== to ========== Miscellaneous fixes to DataUse UI - Plumb the notification that user clicked "Continue" when the dialog box with warning was shown from Java code to native - Java code now queries if a navigation would end data use tracking. This is a simple query which does not change any state (Previously it used a wrong function which would change the internal state of the Tab Model) - Use FrameDeleted instead of RenderFrameDeleted. The former is the one we want to use - Move the snackbar to first paint. Currently, it is called on page load which is too early (underlying code is not aware of navigation at that point) There are still some TODOs which would be fixed in next CL (After everything has moved to UI thread). BUG=570885,558161,552133 ==========
Description was changed from ========== Miscellaneous fixes to DataUse UI - Plumb the notification that user clicked "Continue" when the dialog box with warning was shown from Java code to native - Java code now queries if a navigation would end data use tracking. This is a simple query which does not change any state (Previously it used a wrong function which would change the internal state of the Tab Model) - Use FrameDeleted instead of RenderFrameDeleted. The former is the one we want to use - Move the snackbar to first paint. Currently, it is called on page load which is too early (underlying code is not aware of navigation at that point) There are still some TODOs which would be fixed in next CL (After everything has moved to UI thread). BUG=570885,558161,552133 ========== to ========== Miscellaneous fixes to DataUse UI - Plumb the notification that user clicked "Continue" when the dialog box with warning was shown from Java code to native - Java code now queries if a navigation would end data use tracking. This is a simple query which does not change any state (Previously it used a wrong function which would change the internal state of the Tab Model) - Use FrameDeleted instead of RenderFrameDeleted. The former is the one we want to use - Move the snackbar to first paint. Currently, it is called on page load which is too early (underlying code is not aware of navigation at that point) There are still some TODOs which would be fixed in next CL (After everything has moved to UI thread). BUG=570885,558161,552133 ==========
tbansal@chromium.org changed reviewers: + megjablon@chromium.org
megjablon: PTAL at *. Thanks!
Patchset #1 (id:1) has been deleted
Description was changed from ========== Miscellaneous fixes to DataUse UI - Plumb the notification that user clicked "Continue" when the dialog box with warning was shown from Java code to native - Java code now queries if a navigation would end data use tracking. This is a simple query which does not change any state (Previously it used a wrong function which would change the internal state of the Tab Model) - Use FrameDeleted instead of RenderFrameDeleted. The former is the one we want to use - Move the snackbar to first paint. Currently, it is called on page load which is too early (underlying code is not aware of navigation at that point) There are still some TODOs which would be fixed in next CL (After everything has moved to UI thread). BUG=570885,558161,552133 ========== to ========== Miscellaneous fixes to DataUse UI - Plumb the notification that user clicked "Continue" when the dialog box with warning was shown from Java code to native - Java code now queries if a navigation would end data use tracking. This is a simple query which does not change any state (Previously it used a wrong function which would change the internal state of the Tab Model) - Use FrameDeleted instead of RenderFrameDeleted. The former is the one we want to use - Move the snackbar to first paint. Currently, it is called on page load which is too early (underlying code is not aware of navigation at that point) - Change the order of |url| and |package_name| in custom tab notification (it was reversed when the call went from Java to native). There are still some TODOs which would be fixed in next CL (After everything has moved to UI thread). BUG=570885,558161,552133 ==========
https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:471: public void didFirstVisuallyNonEmptyPaint(Tab tab) { Do we know if waiting for the first paint is ok with the UX people? https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:474: } else if (DataUseTabUIManager.checkDataUseTrackingEnded(tab) The getOptedOutOfDataUseDialog check should come before checkDataUseTrackingEnded https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:83: public static boolean wouldDataUseTrackingEnd( 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? https://codereview.chromium.org/1539763004/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1539763004/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:92: RemoveTabEvent(tab_id, DATA_USE_CONTINUE_CLICKED); Add comment why this is here. https://codereview.chromium.org/1539763004/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:112: RemoveTabEvent(tab_id, DATA_USE_CONTINUE_CLICKED); Won't MaybeCreateTabEvent create the event and return before you get here?
megjablon: ptal. Thanks! https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:471: public void didFirstVisuallyNonEmptyPaint(Tab tab) { On 2015/12/18 20:17:53, megjablon wrote: > Do we know if waiting for the first paint is ok with the UX people? I assume so. It is similar to Lo-Fi. On slow networks, we probably want to show UI closer to when the page starts loading. https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:474: } else if (DataUseTabUIManager.checkDataUseTrackingEnded(tab) On 2015/12/18 20:17:53, megjablon wrote: > The getOptedOutOfDataUseDialog check should come before > checkDataUseTrackingEnded Good point. Done. https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:83: public static boolean wouldDataUseTrackingEnd( On 2015/12/18 20:17:53, megjablon wrote: > 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? 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). https://codereview.chromium.org/1539763004/diff/20001/chrome/browser/android/... File chrome/browser/android/data_usage/data_use_ui_tab_model.cc (right): https://codereview.chromium.org/1539763004/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:92: RemoveTabEvent(tab_id, DATA_USE_CONTINUE_CLICKED); On 2015/12/18 20:17:53, megjablon wrote: > Add comment why this is here. Done. https://codereview.chromium.org/1539763004/diff/20001/chrome/browser/android/... chrome/browser/android/data_usage/data_use_ui_tab_model.cc:112: RemoveTabEvent(tab_id, DATA_USE_CONTINUE_CLICKED); On 2015/12/18 20:17:53, megjablon wrote: > Won't MaybeCreateTabEvent create the event and return before you get here? Added comments in the file. Done. If the state was set to "DATA_USE_CONTINUE_CLICKED", then MaybeCreateEvent would have failed above.
https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:471: public void didFirstVisuallyNonEmptyPaint(Tab tab) { On 2015/12/18 21:35:37, tbansal1 wrote: > On 2015/12/18 20:17:53, megjablon wrote: > > Do we know if waiting for the first paint is ok with the UX people? > > I assume so. It is similar to Lo-Fi. On slow networks, we probably want to show > UI closer to when the page starts loading. Acknowledged. https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:83: public static boolean wouldDataUseTrackingEnd( On 2015/12/18 21:35:37, tbansal1 wrote: > On 2015/12/18 20:17:53, megjablon wrote: > > 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? > > 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). 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? https://codereview.chromium.org/1539763004/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://codereview.chromium.org/1539763004/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:178: userClickedContinueOnDialogBox(tab); This should be done before loadUrl()
https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:83: public static boolean wouldDataUseTrackingEnd( On 2015/12/18 22:08:10, megjablon wrote: > On 2015/12/18 21:35:37, tbansal1 wrote: > > On 2015/12/18 20:17:53, megjablon wrote: > > > 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? > > > > 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). > > 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? 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.
https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:83: public static boolean wouldDataUseTrackingEnd( On 2015/12/18 22:08:10, megjablon wrote: > On 2015/12/18 21:35:37, tbansal1 wrote: > > On 2015/12/18 20:17:53, megjablon wrote: > > > 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? > > > > 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). > > 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? 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.
Description was changed from ========== Miscellaneous fixes to DataUse UI - Plumb the notification that user clicked "Continue" when the dialog box with warning was shown from Java code to native - Java code now queries if a navigation would end data use tracking. This is a simple query which does not change any state (Previously it used a wrong function which would change the internal state of the Tab Model) - Use FrameDeleted instead of RenderFrameDeleted. The former is the one we want to use - Move the snackbar to first paint. Currently, it is called on page load which is too early (underlying code is not aware of navigation at that point) - Change the order of |url| and |package_name| in custom tab notification (it was reversed when the call went from Java to native). There are still some TODOs which would be fixed in next CL (After everything has moved to UI thread). BUG=570885,558161,552133 ========== to ========== Miscellaneous fixes to DataUse UI - Plumb the notification that user clicked "Continue" when the dialog box with warning was shown from Java code to native - Java code now queries if a navigation would end data use tracking. This is a simple query which does not change any state (Previously it used a wrong function which would change the internal state of the Tab Model) - Use FrameDeleted instead of RenderFrameDeleted. The former is the one we want to use - Move the snackbar to first paint. Currently, it is called on page load which is too early (underlying code is not aware of navigation at that point) - Change the order of |url| and |package_name| in custom tab notification (it was reversed when the call went from Java to native). BUG=570885,558161,552133 ==========
tbansal@chromium.org changed reviewers: + rajendrant@chromium.org
Finished the TODOs since all the blocking CLs have landed. Adding rajendrant@ as a reviewer too. Thanks.
Patchset #6 (id:120001) has been deleted
https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:83: public static boolean wouldDataUseTrackingEnd( On 2015/12/18 22:33:09, tbansal1 wrote: > On 2015/12/18 22:08:10, megjablon wrote: > > On 2015/12/18 21:35:37, tbansal1 wrote: > > > On 2015/12/18 20:17:53, megjablon wrote: > > > > 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? > > > > > > 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). > > > > 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? > > 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. 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. https://codereview.chromium.org/1539763004/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1539763004/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:85: // warning was shown. Includes the |tab_id| on which the warning was shown. Add to comment the reason this is necessary.
ptal. thanks. https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://codereview.chromium.org/1539763004/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:83: public static boolean wouldDataUseTrackingEnd( On 2015/12/28 20:32:06, megjablon wrote: > On 2015/12/18 22:33:09, tbansal1 wrote: > > On 2015/12/18 22:08:10, megjablon wrote: > > > On 2015/12/18 21:35:37, tbansal1 wrote: > > > > On 2015/12/18 20:17:53, megjablon wrote: > > > > > 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? > > > > > > > > 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). > > > > > > 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? > > > > 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. > > 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. 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. https://codereview.chromium.org/1539763004/diff/140001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1539763004/diff/140001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:85: // warning was shown. Includes the |tab_id| on which the warning was shown. On 2015/12/28 20:32:06, megjablon wrote: > Add to comment the reason this is necessary. Done.
https://chromiumcodereview.appspot.com/1539763004/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://chromiumcodereview.appspot.com/1539763004/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:83: public static boolean wouldDataUseTrackingEnd( On 2015/12/28 23:55:52, tbansal1 wrote: > On 2015/12/28 20:32:06, megjablon wrote: > > On 2015/12/18 22:33:09, tbansal1 wrote: > > > On 2015/12/18 22:08:10, megjablon wrote: > > > > On 2015/12/18 21:35:37, tbansal1 wrote: > > > > > On 2015/12/18 20:17:53, megjablon wrote: > > > > > > 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? > > > > > > > > > > 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). > > > > > > > > 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? > > > > > > 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. > > > > 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. > > 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. 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?
ptal. https://chromiumcodereview.appspot.com/1539763004/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://chromiumcodereview.appspot.com/1539763004/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:83: public static boolean wouldDataUseTrackingEnd( On 2015/12/29 00:05:21, megjablon wrote: > On 2015/12/28 23:55:52, tbansal1 wrote: > > On 2015/12/28 20:32:06, megjablon wrote: > > > On 2015/12/18 22:33:09, tbansal1 wrote: > > > > On 2015/12/18 22:08:10, megjablon wrote: > > > > > On 2015/12/18 21:35:37, tbansal1 wrote: > > > > > > On 2015/12/18 20:17:53, megjablon wrote: > > > > > > > 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? > > > > > > > > > > > > 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). > > > > > > > > > > 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? > > > > > > > > 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. > > > > > > 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. > > > > 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. > > 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? 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.
https://chromiumcodereview.appspot.com/1539763004/diff/20001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://chromiumcodereview.appspot.com/1539763004/diff/20001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:83: public static boolean wouldDataUseTrackingEnd( On 2015/12/29 00:09:35, tbansal1 wrote: > On 2015/12/29 00:05:21, megjablon wrote: > > On 2015/12/28 23:55:52, tbansal1 wrote: > > > On 2015/12/28 20:32:06, megjablon wrote: > > > > On 2015/12/18 22:33:09, tbansal1 wrote: > > > > > On 2015/12/18 22:08:10, megjablon wrote: > > > > > > On 2015/12/18 21:35:37, tbansal1 wrote: > > > > > > > On 2015/12/18 20:17:53, megjablon wrote: > > > > > > > > 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? > > > > > > > > > > > > > > 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). > > > > > > > > > > > > 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? > > > > > > > > > > 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. > > > > > > > > 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. > > > > > > 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. > > > > 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? > > 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. Ok cool, SGTM! https://chromiumcodereview.appspot.com/1539763004/diff/160001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://chromiumcodereview.appspot.com/1539763004/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:75: * navigation continues. Add "Should only be called before the navigation starts." https://chromiumcodereview.appspot.com/1539763004/diff/160001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:130: && wouldDataUseTrackingEnd(tab, url, pageTransitionType, "")) { Why have the package name param if you're just using ""? https://chromiumcodereview.appspot.com/1539763004/diff/160001/chrome/browser/... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://chromiumcodereview.appspot.com/1539763004/diff/160001/chrome/browser/... chrome/browser/android/data_usage/data_use_ui_tab_model.h:79: // would end tracking of data use. Add "Should only be called before the navigation starts." https://chromiumcodereview.appspot.com/1539763004/diff/160001/chrome/browser/... File chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc (right): https://chromiumcodereview.appspot.com/1539763004/diff/160001/chrome/browser/... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:229: TEST_F(DataUseUITabModelTest, EntranceExitStateWithDialogBox) { Maybe name EntraceExitStateForDialog?
ptal. thanks. https://codereview.chromium.org/1539763004/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://codereview.chromium.org/1539763004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:75: * navigation continues. On 2015/12/29 00:38:24, megjablon wrote: > Add "Should only be called before the navigation starts." Done. https://codereview.chromium.org/1539763004/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:130: && wouldDataUseTrackingEnd(tab, url, pageTransitionType, "")) { On 2015/12/29 00:38:24, megjablon wrote: > Why have the package name param if you're just using ""? Good point. Done. https://codereview.chromium.org/1539763004/diff/160001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model.h (right): https://codereview.chromium.org/1539763004/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model.h:79: // would end tracking of data use. On 2015/12/29 00:38:24, megjablon wrote: > Add "Should only be called before the navigation starts." Done. https://codereview.chromium.org/1539763004/diff/160001/chrome/browser/android... File chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc (right): https://codereview.chromium.org/1539763004/diff/160001/chrome/browser/android... chrome/browser/android/data_usage/data_use_ui_tab_model_unittest.cc:229: TEST_F(DataUseUITabModelTest, EntranceExitStateWithDialogBox) { On 2015/12/29 00:38:24, megjablon wrote: > Maybe name EntraceExitStateForDialog? Done.
lgtm
tbansal@chromium.org changed reviewers: + newt@chromium.org
newt: PTAL at ChromeActivity.java. Thanks.
Description was changed from ========== Miscellaneous fixes to DataUse UI - Plumb the notification that user clicked "Continue" when the dialog box with warning was shown from Java code to native - Java code now queries if a navigation would end data use tracking. This is a simple query which does not change any state (Previously it used a wrong function which would change the internal state of the Tab Model) - Use FrameDeleted instead of RenderFrameDeleted. The former is the one we want to use - Move the snackbar to first paint. Currently, it is called on page load which is too early (underlying code is not aware of navigation at that point) - Change the order of |url| and |package_name| in custom tab notification (it was reversed when the call went from Java to native). BUG=570885,558161,552133 ========== to ========== Miscellaneous fixes to DataUse UI - Plumb the notification that user clicked "Continue" when the dialog box with warning was shown from Java code to native - Java code now queries if a navigation would end data use tracking. This is a simple query which does not change any state (Previously it used a wrong function which would change the internal state of the Tab Model) - Use FrameDeleted instead of RenderFrameDeleted. The former is the one we want to use - Move the snackbar to onDidNavigateMainFrame. Currently, it is called on page load which is too early (underlying code is not aware of navigation at that point) - Change the order of |url| and |package_name| in custom tab notification (it was reversed when the call went from Java to native). BUG=570885,558161,552133 ==========
Patchset #9 (id:200001) has been deleted
one comment, then ChromeActivity lgtm. https://chromiumcodereview.appspot.com/1539763004/diff/180001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://chromiumcodereview.appspot.com/1539763004/diff/180001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:95: public static boolean checkDataUseTrackingEnded(Tab tab) { This method name is misleading and should be fixed: the name implies that this method just returns some internal state, but in fact, this method changes that internal state as well. I think you need a verb that makes it clear that this modifies internal state. Perhaps "checkAndAck...". Same goes for "checkDataUseTrackingStarted()"
Patchset #9 (id:220001) has been deleted
https://codereview.chromium.org/1539763004/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://codereview.chromium.org/1539763004/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:95: public static boolean checkDataUseTrackingEnded(Tab tab) { On 2016/01/04 21:39:56, newt wrote: > This method name is misleading and should be fixed: the name implies that this > method just returns some internal state, but in fact, this method changes that > internal state as well. I think you need a verb that makes it clear that this > modifies internal state. Perhaps "checkAndAck...". > > Same goes for "checkDataUseTrackingStarted()" > Done.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from megjablon@chromium.org, newt@chromium.org Link to the patchset: https://codereview.chromium.org/1539763004/#ps260001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1539763004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1539763004/260001
Message was sent while issue was closed.
Description was changed from ========== Miscellaneous fixes to DataUse UI - Plumb the notification that user clicked "Continue" when the dialog box with warning was shown from Java code to native - Java code now queries if a navigation would end data use tracking. This is a simple query which does not change any state (Previously it used a wrong function which would change the internal state of the Tab Model) - Use FrameDeleted instead of RenderFrameDeleted. The former is the one we want to use - Move the snackbar to onDidNavigateMainFrame. Currently, it is called on page load which is too early (underlying code is not aware of navigation at that point) - Change the order of |url| and |package_name| in custom tab notification (it was reversed when the call went from Java to native). BUG=570885,558161,552133 ========== to ========== Miscellaneous fixes to DataUse UI - Plumb the notification that user clicked "Continue" when the dialog box with warning was shown from Java code to native - Java code now queries if a navigation would end data use tracking. This is a simple query which does not change any state (Previously it used a wrong function which would change the internal state of the Tab Model) - Use FrameDeleted instead of RenderFrameDeleted. The former is the one we want to use - Move the snackbar to onDidNavigateMainFrame. Currently, it is called on page load which is too early (underlying code is not aware of navigation at that point) - Change the order of |url| and |package_name| in custom tab notification (it was reversed when the call went from Java to native). BUG=570885,558161,552133 ==========
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== Miscellaneous fixes to DataUse UI - Plumb the notification that user clicked "Continue" when the dialog box with warning was shown from Java code to native - Java code now queries if a navigation would end data use tracking. This is a simple query which does not change any state (Previously it used a wrong function which would change the internal state of the Tab Model) - Use FrameDeleted instead of RenderFrameDeleted. The former is the one we want to use - Move the snackbar to onDidNavigateMainFrame. Currently, it is called on page load which is too early (underlying code is not aware of navigation at that point) - Change the order of |url| and |package_name| in custom tab notification (it was reversed when the call went from Java to native). BUG=570885,558161,552133 ========== to ========== Miscellaneous fixes to DataUse UI - Plumb the notification that user clicked "Continue" when the dialog box with warning was shown from Java code to native - Java code now queries if a navigation would end data use tracking. This is a simple query which does not change any state (Previously it used a wrong function which would change the internal state of the Tab Model) - Use FrameDeleted instead of RenderFrameDeleted. The former is the one we want to use - Move the snackbar to onDidNavigateMainFrame. Currently, it is called on page load which is too early (underlying code is not aware of navigation at that point) - Change the order of |url| and |package_name| in custom tab notification (it was reversed when the call went from Java to native). BUG=570885,558161,552133 Committed: https://crrev.com/26f2eb746b39256c3f00eeacc360e7206415555d Cr-Commit-Position: refs/heads/master@{#367414} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/26f2eb746b39256c3f00eeacc360e7206415555d Cr-Commit-Position: refs/heads/master@{#367414} |
