Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1804723002:
[Offline pages] Showing offline bolt on chrome custom tab (Closed)
Description[Offline pages] Showing offline bolt on chrome custom tab
* Adds ability to display an offline bolt icon in CCT, when viewing an
offline copy of the page
* Appropriately animates 6 state transitions between the following 3
states: no icon, security icon, offline icon.
BUG=593833
R=tedchoc@chromium.org
Committed: https://crrev.com/d12ae3078333c58fbd49f3e6b0ab3ae0724bab61
Cr-Commit-Position: refs/heads/master@{#381578}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Simplifying per ianwen@'s suggestion #Patch Set 3 : Checking for offline status of a page changing before security update #
Total comments: 2
Patch Set 4 : Addressing feedback" #
Total comments: 1
Patch Set 5 : moving call to show bolt to onNavigatedToDifferentPage #Patch Set 6 : Rebasing #Messages
Total messages: 19 (6 generated)
Description was changed from ========== [Offline pages] Showing offline bolt on chrome custom tab * Adds ability to display an offline bolt icon in CCT, when viewing an offline copy of the page * Appropriately animates 6 state transitions between the following 3 states: no icon, security icon, offline icon. BUG=593833 R=tedchoc@chromium.org ========== to ========== [Offline pages] Showing offline bolt on chrome custom tab * Adds ability to display an offline bolt icon in CCT, when viewing an offline copy of the page * Appropriately animates 6 state transitions between the following 3 states: no icon, security icon, offline icon. BUG=593833 R=tedchoc@chromium.org ==========
Please take a look. I would like to test it, but the test app for Chrome Custom Tab I have cannot be pointed at my trunk build. Also I am piggybacking on updateSecurityIcon for setting the offline icon, which is an unfortunate name, but let's me get away with a pretty simple piece of code. Please advice if there is some better way to express it. thanks. filip
fgorski@chromium.org changed reviewers: + yusufo@chromium.org - tedchoc@chromium.org
ianwen@chromium.org changed reviewers: + ianwen@chromium.org
Q: is it possible that we want to show a security icon and an offline page bolt at the same time? If it is not possible, I would simply give security imageView a different drawable, and leave the animation delegate as is. Then you only need to modify the onSecurityStateChanged() method. If you don't like my first suggestion, another way to simplify the code is to use a ViewSwitcher, which gives you cross-fading animation for free if you want to have a nice transition from a red security alert to a offline page bolt. https://codereview.chromium.org/1804723002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/custom_tabs_toolbar.xml (right): https://codereview.chromium.org/1804723002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/custom_tabs_toolbar.xml:25: android:contentDescription="@string/page_info_offline_icon" Content description is wrong.
Ian, PTAL and generate a binary for me. https://codereview.chromium.org/1804723002/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/custom_tabs_toolbar.xml (right): https://codereview.chromium.org/1804723002/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/custom_tabs_toolbar.xml:25: android:contentDescription="@string/page_info_offline_icon" On 2016/03/14 20:51:56, Ian Wen wrote: > Content description is wrong. Gone.
Updated and manually tested. Yusuf, this is ready for your review.
https://codereview.chromium.org/1804723002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (right): https://codereview.chromium.org/1804723002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:444: Drawable bolt = You can use a TintDrawable for this if the light and dark versions are simply white and grey versions of the same icon. That takes care of the tinting. See the updateVisualsForState call in this class and how we handle tinting. (You might not need to do anything other than setting the resource here.)
Updated. PTAL https://codereview.chromium.org/1804723002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (right): https://codereview.chromium.org/1804723002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:444: Drawable bolt = On 2016/03/15 21:43:44, Yusuf wrote: > You can use a TintDrawable for this if the light and dark versions are simply > white and grey versions of the same icon. That takes care of the tinting. See > the updateVisualsForState call in this class and how we handle tinting. (You > might not need to do anything other than setting the resource here.) Done.
ping
https://codereview.chromium.org/1804723002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java (left): https://codereview.chromium.org/1804723002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/toolbar/CustomTabToolbar.java:430: public void updateSecurityIcon(int securityLevel) { one thing that confuses me about having this here is the frequency of this checks happening. We call this at every progress update, where as the page being offline will be a much less frequent event. Cant we have these checks in a showOfflineBoltIfNecessary() call and call it on onNavigatedToDifferentPage only? I know the checks will keep early returning, better to have it in the right place to begin with.
How about this?
lgtm
The CQ bit was checked by fgorski@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1804723002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804723002/100001
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Showing offline bolt on chrome custom tab * Adds ability to display an offline bolt icon in CCT, when viewing an offline copy of the page * Appropriately animates 6 state transitions between the following 3 states: no icon, security icon, offline icon. BUG=593833 R=tedchoc@chromium.org ========== to ========== [Offline pages] Showing offline bolt on chrome custom tab * Adds ability to display an offline bolt icon in CCT, when viewing an offline copy of the page * Appropriately animates 6 state transitions between the following 3 states: no icon, security icon, offline icon. BUG=593833 R=tedchoc@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== [Offline pages] Showing offline bolt on chrome custom tab * Adds ability to display an offline bolt icon in CCT, when viewing an offline copy of the page * Appropriately animates 6 state transitions between the following 3 states: no icon, security icon, offline icon. BUG=593833 R=tedchoc@chromium.org ========== to ========== [Offline pages] Showing offline bolt on chrome custom tab * Adds ability to display an offline bolt icon in CCT, when viewing an offline copy of the page * Appropriately animates 6 state transitions between the following 3 states: no icon, security icon, offline icon. BUG=593833 R=tedchoc@chromium.org Committed: https://crrev.com/d12ae3078333c58fbd49f3e6b0ab3ae0724bab61 Cr-Commit-Position: refs/heads/master@{#381578} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/d12ae3078333c58fbd49f3e6b0ab3ae0724bab61 Cr-Commit-Position: refs/heads/master@{#381578} |
