Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1418473005:
Add a dialog UI for Data Use accounting (Closed)
DescriptionAdd a dialog UI for Data Use accounting
Creates a dialog for data use accounting that notifies a user when
data use tracking has finished. It is launched via the
InterceptNavigationDelegate. InterceptNavigationDelegate asks the
DataUseTabUIManager whether the navigation should be overriden. If
the navigation should be overriden, the DataUseTabUIManager shows a
dialog. The dialog allows the user to "continue", which restarts the
load, cancel, and opt out of seeing the dialog again. If the user
opts out of the dialog, he or she will see a snackbar instead from
then on.
BUG=551192
Committed: https://crrev.com/45bc5edfdb3aba62aada1481778e1ccc2fc5edf4
Cr-Commit-Position: refs/heads/master@{#358711}
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 2
Patch Set 3 : remove we #
Total comments: 33
Patch Set 4 : newt comments #
Total comments: 3
Patch Set 5 : nit #Patch Set 6 : rename #Patch Set 7 : rebase #Patch Set 8 : adding OWNERS back #Messages
Total messages: 30 (15 generated)
Description was changed from ========== TODO tomorrow BUG= ========== to ========== Add a dialog UI for Data Use accounting Creates a dialog for data use accounting that notifies a user when data use tracking has finished. It is launched via the InterceptNavigationDelegate. InterceptNavigationDelegate asks the DataUseTabUIManager whether the navigation should be overriden. If the navigation should be overriden, the DataUseTabUIManager shows a dialog. The dialog allows the user to "continue", which restarts the load, cancel, and opt out of seeing the dialog again. If the user opts out of the dialog, he or she will see a snackbar instead from then on. BUG=551192 ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== Add a dialog UI for Data Use accounting Creates a dialog for data use accounting that notifies a user when data use tracking has finished. It is launched via the InterceptNavigationDelegate. InterceptNavigationDelegate asks the DataUseTabUIManager whether the navigation should be overriden. If the navigation should be overriden, the DataUseTabUIManager shows a dialog. The dialog allows the user to "continue", which restarts the load, cancel, and opt out of seeing the dialog again. If the user opts out of the dialog, he or she will see a snackbar instead from then on. BUG=551192 ========== to ========== Add a dialog UI for Data Use accounting Creates a dialog for data use accounting that notifies a user when data use tracking has finished. It is launched via the InterceptNavigationDelegate. InterceptNavigationDelegate asks the DataUseTabUIManager whether the navigation should be overriden. If the navigation should be overriden, the DataUseTabUIManager shows a dialog. The dialog allows the user to "continue", which restarts the load, cancel, and opt out of seeing the dialog again. If the user opts out of the dialog, he or she will see a snackbar instead from then on. BUG=551192 ==========
Patchset #2 (id:2) has been deleted
megjablon@chromium.org changed reviewers: + newt@chromium.org
PTAL
bengr@chromium.org changed reviewers: + bengr@chromium.org
https://codereview.chromium.org/1418473005/diff/50001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/InterceptNavigationDelegateImpl.java (right): https://codereview.chromium.org/1418473005/diff/50001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/InterceptNavigationDelegateImpl.java:44: /* We should override the URL loading and launch an intent. */ Remove "We". I.e., do not personify code.
https://chromiumcodereview.appspot.com/1418473005/diff/50001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/tab/InterceptNavigationDelegateImpl.java (right): https://chromiumcodereview.appspot.com/1418473005/diff/50001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/tab/InterceptNavigationDelegateImpl.java:44: /* We should override the URL loading and launch an intent. */ On 2015/11/05 23:47:01, bengr wrote: > Remove "We". I.e., do not personify code. Sorry just moved this code :/ Done.
Patchset #3 (id:70001) has been deleted
https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... File chrome/android/java/res/layout/data_use_dialog.xml (right): https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/res/layout/data_use_dialog.xml:1: <FrameLayout xmlns:android="http://schemas.android.com/apk/res/android" missing copyright https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/res/layout/data_use_dialog.xml:3: android:layout_width="fill_parent" s/fill_parent/match_parent/ (they mean the same thing, but we use match_parent for consistency) https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/res/layout/data_use_dialog.xml:7: <CheckBox Do you need the FrameLayout? Looks like you could just add padding to the CheckBox and remove the FrameLayout. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/res/layout/data_use_dialog.xml:10: android:layout_width="fill_parent" match_parent https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:40: public static boolean hasDataUseTrackingStarted(Tab tab) { Could you clarify when this is true? Once data use tracking starts, does this stay true for the remaining lifetime of the tab? Does this become false once data use tracking ends? https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:51: public static boolean hasDataUseTrackingEnded(Tab tab) { Could you clarify when this is true? Once data use tracking ends, does this stay true for the remaining lifetime of the tab? https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:61: * @param activity current activity. We usually capitalize the first letter in the @param description, e.g. @param activity Current activity. It's not a strict rule, but I think it's easier to read. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:69: public static OverrideUrlLoadingResult shouldOverrideUrlLoading(ChromeActivity activity, s/ChromeActivity/Activity/ since you're not depending on anything that's specific to ChromeActivity https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:74: return OverrideUrlLoadingResult.OVERRIDE_WITH_EXTERNAL_INTENT; Why "EXTERNAL_INTENT"? An external intent is something like launching the YouTube app instead of navigating to youtube.com. In this case, we're showing a dialog, which is not an external intent at all. Returning OVERRIDE_WITH_EXTERNAL_INTENT will cause the InterceptNavigationDelegateImpl to try to launch an external intent, which doesn't make any sense here. I would just make this method return a boolean. (and then you don't need to move OverrideUrlLoadingResult into InterceptNavigationDelegateImpl) https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:90: private static void startDataUseDialogIntent(final ChromeActivity activity, final Tab tab, This doesn't involve intents. I'd remove "Intent" from the method name. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:97: setOptedOutOfDataUseDialog(activity, isChecked); I think we should call setOptedOutOfDataUseDialog() when the user clicks "OK" on the dialog, rather than when they check the checkbox. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/tab/InterceptNavigationDelegateImpl.java (right): https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/tab/InterceptNavigationDelegateImpl.java:128: result = DataUseTabUIManager.shouldOverrideUrlLoading(mActivity, mTab, url, Do we want to warn users if they're leaving Chrome and the data use tracking session is ending? E.g. if they navigate to youtube.com and this code redirects them to the YouTube app? If so, then I'd move DataUseTabUIManager.shouldOverrideUrlLoading() up to line 100. Also, does this work correctly with tabs opened in the background? Or in document mode? https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... File chrome/android/java/strings/android_chrome_strings.grd (right): https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/strings/android_chrome_strings.grd:792: <message name="IDS_DATA_USE_TRACKING_ENDED_CHECKBOX_MESSAGE" desc="Message shown to opt out of seeing dialogs when data use tracking has ended. The data use ended snackbar will be shown instead from then on."> These descriptions are for translators. Include details that they'll need, but leave out irrelevant parts, such as "The data use ended snackbar will be shown instead from then on." https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/strings/android_chrome_strings.grd:793: Don't ask me again use a curly apostrophe (’) instead of a straight apostrophe (')
https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:90: private static void startDataUseDialogIntent(final ChromeActivity activity, final Tab tab, On 2015/11/06 20:40:36, newt wrote: > This doesn't involve intents. I'd remove "Intent" from the method name. probably just "showDataUseDialog()"
https://codereview.chromium.org/1418473005/diff/90001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://codereview.chromium.org/1418473005/diff/90001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:57: * Returns whether a navigation should be paused to show a dialog telling the user that data use This is a wonderfully clear description, by the way. Thank you :)
https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... File chrome/android/java/res/layout/data_use_dialog.xml (right): https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/res/layout/data_use_dialog.xml:1: <FrameLayout xmlns:android="http://schemas.android.com/apk/res/android" On 2015/11/06 20:40:36, newt wrote: > missing copyright Done. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/res/layout/data_use_dialog.xml:3: android:layout_width="fill_parent" On 2015/11/06 20:40:36, newt wrote: > s/fill_parent/match_parent/ (they mean the same thing, but we use match_parent > for consistency) Done. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/res/layout/data_use_dialog.xml:7: <CheckBox On 2015/11/06 20:40:36, newt wrote: > Do you need the FrameLayout? Looks like you could just add padding to the > CheckBox and remove the FrameLayout. Adding the padding to the checkbox directly moves the text placement, but not checkbox. Using layout_margin seems to be overriden when setting the view. I could remove the FrameLayout and use the setView that takes spacing values, but that would mean calculating the dp to px values when the dialog is displayed and this seems simpler. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/res/layout/data_use_dialog.xml:10: android:layout_width="fill_parent" On 2015/11/06 20:40:36, newt wrote: > match_parent Done. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:40: public static boolean hasDataUseTrackingStarted(Tab tab) { On 2015/11/06 20:40:36, newt wrote: > Could you clarify when this is true? Once data use tracking starts, does this > stay true for the remaining lifetime of the tab? Does this become false once > data use tracking ends? Done. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:51: public static boolean hasDataUseTrackingEnded(Tab tab) { On 2015/11/06 20:40:36, newt wrote: > Could you clarify when this is true? Once data use tracking ends, does this stay > true for the remaining lifetime of the tab? Done. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:57: * Returns whether a navigation should be paused to show a dialog telling the user that data use On 2015/11/06 20:45:44, newt wrote: > This is a wonderfully clear description, by the way. Thank you :) :) https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:61: * @param activity current activity. On 2015/11/06 20:40:36, newt wrote: > We usually capitalize the first letter in the @param description, e.g. @param > activity Current activity. It's not a strict rule, but I think it's easier to > read. Agreed. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:69: public static OverrideUrlLoadingResult shouldOverrideUrlLoading(ChromeActivity activity, On 2015/11/06 20:40:36, newt wrote: > s/ChromeActivity/Activity/ since you're not depending on anything that's > specific to ChromeActivity Done. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:74: return OverrideUrlLoadingResult.OVERRIDE_WITH_EXTERNAL_INTENT; On 2015/11/06 20:40:36, newt wrote: > Why "EXTERNAL_INTENT"? An external intent is something like launching the > YouTube app instead of navigating to http://youtube.com. In this case, we're showing a > dialog, which is not an external intent at all. > > Returning OVERRIDE_WITH_EXTERNAL_INTENT will cause the > InterceptNavigationDelegateImpl to try to launch an external intent, which > doesn't make any sense here. > > I would just make this method return a boolean. (and then you don't need to move > OverrideUrlLoadingResult into InterceptNavigationDelegateImpl) Done. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:90: private static void startDataUseDialogIntent(final ChromeActivity activity, final Tab tab, On 2015/11/06 20:40:36, newt wrote: > This doesn't involve intents. I'd remove "Intent" from the method name. Done. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:97: setOptedOutOfDataUseDialog(activity, isChecked); On 2015/11/06 20:40:36, newt wrote: > I think we should call setOptedOutOfDataUseDialog() when the user clicks "OK" on > the dialog, rather than when they check the checkbox. Done. What is the standard for if the user clicks outside of the dialog? Should I setCancelable(false) so the user has to choose one of the buttons, or is that a valid means of exiting and I should set the checkbox state? https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/tab/InterceptNavigationDelegateImpl.java (right): https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/tab/InterceptNavigationDelegateImpl.java:128: result = DataUseTabUIManager.shouldOverrideUrlLoading(mActivity, mTab, url, On 2015/11/06 20:40:36, newt wrote: > Do we want to warn users if they're leaving Chrome and the data use tracking > session is ending? E.g. if they navigate to http://youtube.com and this code redirects > them to the YouTube app? > > If so, then I'd move DataUseTabUIManager.shouldOverrideUrlLoading() up to line > 100. > > Also, does this work correctly with tabs opened in the background? Or in > document mode? For v1 we're not going to alert the user when he or she goes to another app. This happens on navigation so from what I know we shouldn't have any problems with background tabs. It works properly in document mode. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... File chrome/android/java/strings/android_chrome_strings.grd (right): https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/strings/android_chrome_strings.grd:792: <message name="IDS_DATA_USE_TRACKING_ENDED_CHECKBOX_MESSAGE" desc="Message shown to opt out of seeing dialogs when data use tracking has ended. The data use ended snackbar will be shown instead from then on."> On 2015/11/06 20:40:36, newt wrote: > These descriptions are for translators. Include details that they'll need, but > leave out irrelevant parts, such as "The data use ended snackbar will be shown > instead from then on." Done. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/strings/android_chrome_strings.grd:793: Don't ask me again On 2015/11/06 20:40:36, newt wrote: > use a curly apostrophe (’) instead of a straight apostrophe (') Done.
Patchset #4 (id:110001) has been deleted
lgtm after nit https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... File chrome/android/java/res/layout/data_use_dialog.xml (right): https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/res/layout/data_use_dialog.xml:7: <CheckBox On 2015/11/06 23:41:40, megjablon wrote: > On 2015/11/06 20:40:36, newt wrote: > > Do you need the FrameLayout? Looks like you could just add padding to the > > CheckBox and remove the FrameLayout. > > Adding the padding to the checkbox directly moves the text placement, but not > checkbox. Using layout_margin seems to be overriden when setting the view. I > could remove the FrameLayout and use the setView that takes spacing values, but > that would mean calculating the dp to px values when the dialog is displayed and > this seems simpler. Ah, I was worried that padding might interfere with checkbox positioning. Just keep the FrameLayout as it is. https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://chromiumcodereview.appspot.com/1418473005/diff/90001/chrome/android/j... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:97: setOptedOutOfDataUseDialog(activity, isChecked); On 2015/11/06 23:41:40, megjablon wrote: > On 2015/11/06 20:40:36, newt wrote: > > I think we should call setOptedOutOfDataUseDialog() when the user clicks "OK" > on > > the dialog, rather than when they check the checkbox. > > Done. What is the standard for if the user clicks outside of the dialog? Should > I setCancelable(false) so the user has to choose one of the buttons, or is that > a valid means of exiting and I should set the checkbox state? If they tap outside the dialog, or press back to exit the dialog, I don't think we should save the checkbox state. I think the current code is good. https://chromiumcodereview.appspot.com/1418473005/diff/130001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java (right): https://chromiumcodereview.appspot.com/1418473005/diff/130001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/datausage/DataUseTabUIManager.java:89: private static void startDataUseDialog(final Activity activity, final Tab tab, s/start/show/ ? https://chromiumcodereview.appspot.com/1418473005/diff/130001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java (right): https://chromiumcodereview.appspot.com/1418473005/diff/130001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:46: * Result types for checking if we should override URL loading. nit: extra space
Patchset #5 (id:150001) has been deleted
Patchset #5 (id:170001) has been deleted
https://chromiumcodereview.appspot.com/1418473005/diff/130001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java (right): https://chromiumcodereview.appspot.com/1418473005/diff/130001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:46: * Result types for checking if we should override URL loading. On 2015/11/07 00:43:34, newt wrote: > nit: extra space Done.
lgtm
newt@chromium.org changed reviewers: + dfalcantara@chromium.org
+dfalcantara to take a look at InterceptNavigationDelegateImpl.java
Patchset #7 (id:230001) has been deleted
Patchset #8 (id:270001) has been deleted
Explicitly not giving an l-g-t-m because I haven't had enough time to give a review and work through all the vagaries of the InterceptNavigateDelegateImpl details with document mode. You have OWNERs approval already, so submit if you're confident that you're extending the InterceptNavigateDelegateImpl directly.
The CQ bit was checked by megjablon@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org, bengr@chromium.org Link to the patchset: https://chromiumcodereview.appspot.com/1418473005/#ps290001 (title: "adding OWNERS back")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1418473005/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1418473005/290001
Message was sent while issue was closed.
Committed patchset #8 (id:290001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/45bc5edfdb3aba62aada1481778e1ccc2fc5edf4 Cr-Commit-Position: refs/heads/master@{#358711} |
