Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2315953002:
Add theme color for Blimp on Android. (Closed)
|
Created:
4 years, 3 months ago by xingliu Modified:
4 years, 3 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd theme color for Blimp on Android.
Use Material Design color Light Blue 100 for a Blimp tab.
See https://material.google.com/style/color.html for details.
BUG=64425
Committed: https://crrev.com/b2e5b44429452386be1908bdad73ed5df0fa0b14
Cr-Commit-Position: refs/heads/master@{#417079}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Added valid color check, add crbug todo. #Patch Set 3 : Added comment on the color. #
Messages
Total messages: 24 (14 generated)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xingliu@chromium.org changed reviewers: + dtrainor@chromium.org, nyquist@chromium.org
Please help to review the theme color patch.
Description was changed from ========== Add theme color for Blimp on Android. Use light blue color for a Blimp tab. BUG=64425 ========== to ========== Add theme color for Blimp on Android. Use Material Design color Light Blue 100 color for a Blimp tab. See https://material.google.com/style/color.html#color-color-palette BUG=64425 ==========
Description was changed from ========== Add theme color for Blimp on Android. Use Material Design color Light Blue 100 color for a Blimp tab. See https://material.google.com/style/color.html#color-color-palette BUG=64425 ========== to ========== Add theme color for Blimp on Android. Use Material Design color Light Blue 100 for a Blimp tab. See https://material.google.com/style/color.html#color-color-palette BUG=64425 ==========
Description was changed from ========== Add theme color for Blimp on Android. Use Material Design color Light Blue 100 for a Blimp tab. See https://material.google.com/style/color.html#color-color-palette BUG=64425 ========== to ========== Add theme color for Blimp on Android. Use Material Design color Light Blue 100 for a Blimp tab. See https://material.google.com/style/color.html for details. BUG=64425 ==========
The CQ bit was unchecked by nyquist@chromium.org
The CQ bit was checked by nyquist@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
https://codereview.chromium.org/2315953002/diff/1/blimp/client/core/contents/... File blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/BlimpContentsImpl.java (right): https://codereview.chromium.org/2315953002/diff/1/blimp/client/core/contents/... blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/BlimpContentsImpl.java:79: return BLIMP_THEME_COLOR; Add a TODO to get the right theme color when we don't need a clear Blimp indicator. https://codereview.chromium.org/2315953002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2315953002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1119: themeColor = getBlimpContents().getThemeColor(); What does isValidThemeColor do? Do we need that?
https://codereview.chromium.org/2315953002/diff/1/blimp/client/core/contents/... File blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/BlimpContentsImpl.java (right): https://codereview.chromium.org/2315953002/diff/1/blimp/client/core/contents/... blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/BlimpContentsImpl.java:79: return BLIMP_THEME_COLOR; On 2016/09/07 05:08:52, David Trainor wrote: > Add a TODO to get the right theme color when we don't need a clear Blimp > indicator. I think this TODO also should come with a new crbug that explains what we would want. https://codereview.chromium.org/2315953002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2315953002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1119: themeColor = getBlimpContents().getThemeColor(); On 2016/09/07 05:08:52, David Trainor wrote: > What does isValidThemeColor do? Do we need that? Turns out it checks the HSL lightness value to be < 0.94. I don't think it'd be necessary right now, but if we add it now, we don't have to remember it later when we add support for real theme colors.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Fix based on review. https://codereview.chromium.org/2315953002/diff/1/blimp/client/core/contents/... File blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/BlimpContentsImpl.java (right): https://codereview.chromium.org/2315953002/diff/1/blimp/client/core/contents/... blimp/client/core/contents/android/java/src/org/chromium/blimp/core/contents/BlimpContentsImpl.java:79: return BLIMP_THEME_COLOR; On 2016/09/07 15:11:04, nyquist wrote: > On 2016/09/07 05:08:52, David Trainor wrote: > > Add a TODO to get the right theme color when we don't need a clear Blimp > > indicator. > > I think this TODO also should come with a new crbug that explains what we would > want. Added a todo that links to a crbug. https://codereview.chromium.org/2315953002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2315953002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1119: themeColor = getBlimpContents().getThemeColor(); On 2016/09/07 15:11:04, nyquist wrote: > On 2016/09/07 05:08:52, David Trainor wrote: > > What does isValidThemeColor do? Do we need that? > > Turns out it checks the HSL lightness value to be < 0.94. I don't think it'd be > necessary right now, but if we add it now, we don't have to remember it later > when we add support for real theme colors. Added isValidThemeColor check.
The CQ bit was checked by nyquist@chromium.org
lgtm
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.
Description was changed from ========== Add theme color for Blimp on Android. Use Material Design color Light Blue 100 for a Blimp tab. See https://material.google.com/style/color.html for details. BUG=64425 ========== to ========== Add theme color for Blimp on Android. Use Material Design color Light Blue 100 for a Blimp tab. See https://material.google.com/style/color.html for details. BUG=64425 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add theme color for Blimp on Android. Use Material Design color Light Blue 100 for a Blimp tab. See https://material.google.com/style/color.html for details. BUG=64425 ========== to ========== Add theme color for Blimp on Android. Use Material Design color Light Blue 100 for a Blimp tab. See https://material.google.com/style/color.html for details. BUG=64425 Committed: https://crrev.com/b2e5b44429452386be1908bdad73ed5df0fa0b14 Cr-Commit-Position: refs/heads/master@{#417079} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/b2e5b44429452386be1908bdad73ed5df0fa0b14 Cr-Commit-Position: refs/heads/master@{#417079} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
