Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2130313003:
Dismiss Lo-Fi snackbar on new page loads and tab dismissal (Closed)
DescriptionDismiss Lo-Fi snackbar on new page loads and tab dismissal
BUG=626757
Committed: https://crrev.com/ae59b40c3055d9f5ef53fc0237bd236e4761833e
Cr-Commit-Position: refs/heads/master@{#404524}
Patch Set 1 #
Total comments: 11
Patch Set 2 : addressing comments #Messages
Total messages: 11 (4 generated)
Patchset #1 (id:1) has been deleted
megjablon@chromium.org changed reviewers: + ianwen@chromium.org
PTAL, thanks!
Looks good overall. I have a question and some nits. https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java (right): https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java:77: } nit: remove #76 and add an else statement to wrap #78. :) https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java:88: TabObserver tabObserver = new EmptyTabObserver() { Use mTabObserver here directly. https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java:106: if (isMainFrame) dismissLoFiBar(); Q: what if the user is currently seeing a lofi webpage, and he/she clicks refresh? Will we show the same snackbar immediately after it is dismissed? Will the UI flash? https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java:133: mTab.removeObserver(mTabObserver); Could you please create another method, say "removeTabObserver()", and put #133 and #134 to it? This way you could call removeTabObserver() in onAction() and onDismissNoAction(). https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java:143: dismissLoFiBar(); #143 and #166 are not needed. Snackbar will be dismissed anyway. What you need is to remove the tab observer.
https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java (right): https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java:77: } On 2016/07/08 21:53:00, Ian Wen wrote: > nit: remove #76 and add an else statement to wrap #78. :) Done. https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java:88: TabObserver tabObserver = new EmptyTabObserver() { On 2016/07/08 21:53:00, Ian Wen wrote: > Use mTabObserver here directly. Done. https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java:106: if (isMainFrame) dismissLoFiBar(); On 2016/07/08 21:53:01, Ian Wen wrote: > Q: what if the user is currently seeing a lofi webpage, and he/she clicks > refresh? Will we show the same snackbar immediately after it is dismissed? Will > the UI flash? Yes, we show the same snackbar immediately after it is dismissed. The snackbar smoothly dismisses and then is reshown after the first Lo-Fi image is loaded. This is the desired behavior because the reload should indicate that Lo-Fi mode is on. If you'd like I can send you a video of what this looks like. https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java:133: mTab.removeObserver(mTabObserver); On 2016/07/08 21:53:01, Ian Wen wrote: > Could you please create another method, say "removeTabObserver()", and put #133 > and #134 to it? This way you could call removeTabObserver() in onAction() and > onDismissNoAction(). Done. https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java:143: dismissLoFiBar(); On 2016/07/08 21:53:01, Ian Wen wrote: > #143 and #166 are not needed. Snackbar will be dismissed anyway. What you need > is to remove the tab observer. Done.
The CQ bit was checked by ianwen@chromium.org
lgtm https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java (right): https://codereview.chromium.org/2130313003/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/snackbar/LofiBarController.java:106: if (isMainFrame) dismissLoFiBar(); On 2016/07/08 22:14:17, megjablon wrote: > On 2016/07/08 21:53:01, Ian Wen wrote: > > Q: what if the user is currently seeing a lofi webpage, and he/she clicks > > refresh? Will we show the same snackbar immediately after it is dismissed? > Will > > the UI flash? > > Yes, we show the same snackbar immediately after it is dismissed. The snackbar > smoothly dismisses and then is reshown after the first Lo-Fi image is loaded. > This is the desired behavior because the reload should indicate that Lo-Fi mode > is on. If you'd like I can send you a video of what this looks like. Ok sg.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Dismiss Lo-Fi snackbar on new page loads and tab dismissal BUG=626757 ========== to ========== Dismiss Lo-Fi snackbar on new page loads and tab dismissal BUG=626757 Committed: https://crrev.com/ae59b40c3055d9f5ef53fc0237bd236e4761833e Cr-Commit-Position: refs/heads/master@{#404524} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ae59b40c3055d9f5ef53fc0237bd236e4761833e Cr-Commit-Position: refs/heads/master@{#404524} |
