Separator color should be based on inactive tab color, not frame color |
|||||||||||||||||||||||||||||||||||||||||||||
Issue descriptionInstall https://chrome.google.com/webstore/detail/texture/kebdomfinmgmcinggaaojlckicdaajkm?hl=en . * Tab separators are now nearly invisible * The NTB is very low contrast * Bookmark text is moderately-low contrast We should at least verify that we're computing our contrasting colors against the correct background colors. I suspect tab separators are not begin computed to contrast with the background tab color in the themed case, for example. Fixes beyond that might require addressing bug 856812 . Hard to know without diving deeper into what's going on right now.
,
Jul 13
This is also an issue with the "Puk-Puk" [1] and "Hedgehog in the fog" [2] themes. [1] https://chrome.google.com/webstore/detail/puk-puk/cngkcldnnppckgbmndaccoffaikjbemc [2] https://chrome.google.com/webstore/detail/hedgehog-in-the-fog/haocganpkafanhkfldbbmhcpaelmkejg
,
Jul 16
I did a little investigating here: * Texture doesn't a frame color, so chrome uses the (light) defaults. Adding a frame color fixes the problem (see attached screenshot as an example). * Puk-Puk seems to be doing the best it can right now. The frame image is pretty busy and the new-tab button kinda gets lost. We may want to give it a button background. I filed bug 864203 for that. * The problem with Hedgehog in the Fog is that we always match the new-tab button to the inactive tab text, but in this case the contrast considerations are different in the two cases. Not sure what the correct thing to do in this case is; I'll consult a designer.
,
Jul 16
The last two points should both be addressed by bug 857197. Having a frame color fix texture seems a bit concerning, since we should be computing the tab separator color based on the background tab color rather than the frame color. That also doesn't speak to bookmark text color, but maybe that's OK.
,
Jul 16
I can't speak to what we should be doing, but we are right now calculating the separator color based on the frame color:
constexpr SkAlpha kTabSeparatorAlpha = 0x4D; // 30%
const SkColor frame_color = GetFrameColor();
const SkColor base_color =
color_utils::BlendTowardOppositeLuma(frame_color, SK_AlphaOPAQUE);
return color_utils::AlphaBlend(base_color, frame_color, kTabSeparatorAlpha);
Anyway, I don't think there's any action here for these specific themes.
,
Jul 16
Right, that's the primary thing I wanted this bug to be about. We should be using the inactive tab color (which will match the frame color by default, but not in themed cases; so it should be safe to always use it) instead of the frame color. I still am concerned about whether the bookmark text has sufficient contrast to hit a11y minimums.
,
Jul 16
Oh okay, changing the bug around then. The bookmark text is pretty light, but Texture doesn't define a toolbar color either. Even if it did, we need bug 856812 to solve that.
,
Jul 17
I have to fix this locally anyway for another bug.
,
Jul 21
Here's some of my progress locally, in the form of Dev 3493 vs. local shots of normal and incognito modes for the three themes named above. Some work here was done by other people on other bugs (e.g. Allen putting a circle behind the NTB, or Bret writing min-contrast-guaranteeing functions). Work I did here: * Remove tab shapes when the theme doesn't ask for them * Compute frame and tab colors automatically so we can correctly decide what caption button/text/NTB colors to put atop them * Respect theme-specified colors better * Improve alignment of background images There's also work not visible in these shots, e.g. fixing the NTB background alignment, mirroring the tab background image vertically if it's too short, providing more customizability for themes (kind of necessary to fix some of the issues above), and fixing a number of bugs in inactive windows. Removing the tab shapes mostly meant changing the background tab tint to a no-op by default. This affects a lot of themes in subtle but noticeable ways, e.g. Puk-Puk's background tabs get a little more transparent, and Hedgehog In The Fog's get darker and more saturated. I think this is OK; theme authors who object can always override that tint.
,
Jul 21
Another sample, that shows off better background image alignment, better separator colors, and fixed gaps below the tabs. Unfortunately it also demonstrates some of the downsides of removing tab shapes. In this case the background is very noisy, so even though the autocomputed background tab text color in incognito mode contrasts reasonably well against the darker part of the frame, it's unreadable over the light parts. While it's probably theoretically possible to do better without overlays here, this is a case where the theme author would likely just need to fix incognito mode to provide tab shapes as normal mode does (which would be easy).
,
Jul 23
Issue 864727 has been merged into this issue.
,
Jul 23
I don't think I was clear in the bug that was just duped here. If you look at http://screen/2p4WkXxYbpb.png, you can see that the tabs are 'touching' across the gap near the top.
,
Jul 23
Yes, the separator is not a gap between tabs but a stroke that is drawn on the tabs in the midst of a region where they overlap. I'll take your comment as feedback that, when coupled with the curve above, this looks arguably glitchy (and would probably look less so if we filled in the region between tabs).
,
Jul 23
Ah! That's why I thought it looked 'glitchy' earlier- it looks like there's meant to be a gap, which would be natural- the stroke looks off somehow. Your interpretation is right, it'd look nicer if the region between tabs was filled with the background.
,
Jul 23
,
Jul 24
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a91ffca64e95ec36000ee43296d5025e89da5244 commit a91ffca64e95ec36000ee43296d5025e89da5244 Author: Peter Kasting <pkasting@chromium.org> Date: Tue Jul 24 22:04:38 2018 Make HasCustomColor() match GetColor(). The latter has various transformations around when it queries the underlying the provider and with what constants. Use this behavior in HasCustomColor() too. This ensures callers get consistent behavior in these cases. Along the way this changes a multi-arm conditional to a helper function. That isn't necessary for this patch, but it will make it easy to land subsequent ones that add more such constants. Bug: 862664 Change-Id: I83295902f7c38767c3a051a40e5f195f8e78e4ef Reviewed-on: https://chromium-review.googlesource.com/1147601 Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Cr-Commit-Position: refs/heads/master@{#577703} [modify] https://crrev.com/a91ffca64e95ec36000ee43296d5025e89da5244/chrome/browser/themes/theme_service.cc [modify] https://crrev.com/a91ffca64e95ec36000ee43296d5025e89da5244/chrome/browser/themes/theme_service.h
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8c35e22c3931c280966345bcaadda1c4f8b38e3e commit 8c35e22c3931c280966345bcaadda1c4f8b38e3e Author: Peter Kasting <pkasting@chromium.org> Date: Fri Jul 27 21:06:26 2018 Add background tab colors and inactive images to the theme system. These will eventually be used for three purposes: (1) To allow generating different background tab images from a common tint for the inactive and incognito cases, thus allowing background tabs that blend into the frame for custom themes. (2) To allow autogenerating the background tab colors from the background tab images, thus letting us autocalculate better tab text/separator/etc. colors. (3) To allow theme authors to customize/override this behavior more precisely. Reasons (1) and (2) dictated the design here; reason (3) is basically a free bonus. I thought about not making these resources overrideable by theme authors, but there was some technical reason that turned out to not be easily feasible, and there seems little reason to prevent it. This also allows us to read THEME_TAB_BACKGROUND_INCOGNITO on Mac (Views). This shouldn't result in a visual change on its own. Bug: 862664 Change-Id: Icc57f78fd526d68b83c692fbac669a7371bb38ff Reviewed-on: https://chromium-review.googlesource.com/1147869 Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Mitsuru Oshima <oshima@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Reviewed-by: Allen Bauer <kylixrd@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#578790} [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/app/theme/theme_resources.grd [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/themes/browser_theme_pack_unittest.cc [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/themes/theme_properties.cc [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/themes/theme_properties.h [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/themes/theme_service.cc [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/ui/views/frame/browser_non_client_frame_view.h [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/ui/views/tabs/browser_tab_strip_controller.h [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.h [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/ui/views/tabs/tab_controller.h [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/ui/views/tabs/tab_strip.cc [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/ui/views/tabs/tab_strip.h [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/ui/views/tabs/tab_strip_controller.h [modify] https://crrev.com/8c35e22c3931c280966345bcaadda1c4f8b38e3e/chrome/browser/ui/views/tabs/tab_unittest.cc
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/df84748a54c4ccd92e732c9168a2b3e8925695f5 commit df84748a54c4ccd92e732c9168a2b3e8925695f5 Author: Peter Kasting <pkasting@chromium.org> Date: Fri Jul 27 22:02:51 2018 Compute colors for the frame and tabs from provided images. If theme authors explicitly provide values here, we'll use them; but if they don't, but do provide images, set the colors to be the dominant colors of the images, using an existing K-means algorithm (similar to what we use for computing representative favicon colors etc.). This allows us to rely on those colors later when deciding what color to make tab text, the new tab button, and similar tabstrip/window frame items. This also does a bit of cleanup to the theme pack code, e.g. moving a map used in only one function into that function to make its provenance clearer. I can try to split this apart into more CLs if desired. Bug: 862664 Change-Id: I8c3f15893ad491a6fee8aa5f76ae162cecb2c6fd Reviewed-on: https://chromium-review.googlesource.com/1152517 Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Cr-Commit-Position: refs/heads/master@{#578823} [modify] https://crrev.com/df84748a54c4ccd92e732c9168a2b3e8925695f5/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/df84748a54c4ccd92e732c9168a2b3e8925695f5/chrome/browser/themes/browser_theme_pack.h [modify] https://crrev.com/df84748a54c4ccd92e732c9168a2b3e8925695f5/chrome/browser/themes/browser_theme_pack_unittest.cc [modify] https://crrev.com/df84748a54c4ccd92e732c9168a2b3e8925695f5/ui/gfx/color_analysis.cc [modify] https://crrev.com/df84748a54c4ccd92e732c9168a2b3e8925695f5/ui/gfx/color_analysis.h
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a37ba3b982c557fbebf5de025cf300f9333dce07 commit a37ba3b982c557fbebf5de025cf300f9333dce07 Author: Scott Violet <sky@chromium.org> Date: Fri Jul 27 23:27:25 2018 Revert "Compute colors for the frame and tabs from provided images." This reverts commit df84748a54c4ccd92e732c9168a2b3e8925695f5. Reason for revert: To fix unit_tests (I have to revert this before the earlier patch). Likely suspect for BrowserThemePackTest.HiDpiThemeTest failing on bots. For example, https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/linux-chromeos-rel/11230 : [ RUN ] BrowserThemePackTest.HiDpiThemeTest ../../chrome/browser/themes/browser_theme_pack_unittest.cc:204: Failure Expected equality of these values: "" error Which is: "File doesn't exist." Stack trace: #0 0x0000026103ac testing::internal::UnitTestImpl::CurrentOsStackTraceExceptTop() #1 0x00000260fd89 testing::internal::AssertHelper::operator=() #2 0x000000fc9b98 BrowserThemePackTest::BuildFromUnpackedExtension() #3 0x000000fd22cc BrowserThemePackTest_HiDpiThemeTest_Test::TestBody() ../../chrome/browser/themes/browser_theme_pack_unittest.cc:205: Failure Value of: valid_value.get() Actual: false Expected: true Stack trace: #0 0x0000026103ac testing::internal::UnitTestImpl::CurrentOsStackTraceExceptTop() #1 0x00000260fd89 testing::internal::AssertHelper::operator=() #2 0x000000fc9e0b BrowserThemePackTest::BuildFromUnpackedExtension() #3 0x000000fd22cc BrowserThemePackTest_HiDpiThemeTest_Test::TestBody() Received signal 11 SEGV_MAPERR 000000000028 #0 0x000004d00e1c base::debug::StackTrace::StackTrace() #1 0x000004d00981 base::debug::(anonymous namespace)::StackDumpSignalHandler() #2 0x7f26bba79330 <unknown> #3 0x0000050f3e24 BrowserThemePack::WriteToDisk() #4 0x000000fd22db BrowserThemePackTest_HiDpiThemeTest_Test::TestBody() #5 0x0000026161b2 testing::Test::Run() #6 0x000002616d30 testing::TestInfo::Run() #7 0x000002617247 testing::TestCase::Run() #8 0x000002622747 testing::internal::UnitTestImpl::RunAllTests() #9 0x0000026222bd testing::UnitTest::Run() #10 0x0000045565d1 base::TestSuite::Run() #11 0x000004557fca base::(anonymous namespace)::LaunchUnitTestsInternal() #12 0x000004557e7a base::LaunchUnitTests() #13 0x00000454e155 main #14 0x7f26b86c9f45 __libc_start_main #15 0x0000006fa82a _start r8: 0000000000000000 r9: 54656d6568546970 r10: 747365545f747365 r11: 0000000000000000 r12: 000024a8af0c0d20 r13: 00007fff447924e8 r14: 00007fff44792560 r15: 0000000000000000 di: 0000000000000000 si: 00007fff44792560 bp: 00007fff44792520 bx: 00007fff44792578 dx: 00000000000005e7 ax: 0000000000001fdd cx: 0000000000000023 sp: 00007fff44792490 ip: 00000000050f3e24 efl: 0000000000010206 cgf: 0000000000000033 erf: 0000000000000004 trp: 000000000000000e msk: 0000000000000000 cr2: 0000000000000028 [end of stack trace] Original change's description: > Compute colors for the frame and tabs from provided images. > > If theme authors explicitly provide values here, we'll use them; but if they > don't, but do provide images, set the colors to be the dominant colors of the > images, using an existing K-means algorithm (similar to what we use for > computing representative favicon colors etc.). > > This allows us to rely on those colors later when deciding what color to make > tab text, the new tab button, and similar tabstrip/window frame items. > > This also does a bit of cleanup to the theme pack code, e.g. moving a map used > in only one function into that function to make its provenance clearer. I can > try to split this apart into more CLs if desired. > > Bug: 862664 > Change-Id: I8c3f15893ad491a6fee8aa5f76ae162cecb2c6fd > Reviewed-on: https://chromium-review.googlesource.com/1152517 > Commit-Queue: Peter Kasting <pkasting@chromium.org> > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> > Reviewed-by: Evan Stade <estade@chromium.org> > Cr-Commit-Position: refs/heads/master@{#578823} TBR=pkasting@chromium.org,asvitkine@chromium.org,estade@chromium.org Change-Id: I6a5a7f2916136ccf93101f36c34a17a4e5ef5d4f No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 862664 Reviewed-on: https://chromium-review.googlesource.com/1153988 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#578856} [modify] https://crrev.com/a37ba3b982c557fbebf5de025cf300f9333dce07/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/a37ba3b982c557fbebf5de025cf300f9333dce07/chrome/browser/themes/browser_theme_pack.h [modify] https://crrev.com/a37ba3b982c557fbebf5de025cf300f9333dce07/chrome/browser/themes/browser_theme_pack_unittest.cc [modify] https://crrev.com/a37ba3b982c557fbebf5de025cf300f9333dce07/ui/gfx/color_analysis.cc [modify] https://crrev.com/a37ba3b982c557fbebf5de025cf300f9333dce07/ui/gfx/color_analysis.h
,
Jul 27
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/62a8750bd3812074a383ad3e10bf99d5ed49e93c commit 62a8750bd3812074a383ad3e10bf99d5ed49e93c Author: Scott Violet <sky@chromium.org> Date: Fri Jul 27 23:39:08 2018 Revert "Add background tab colors and inactive images to the theme system." This reverts commit 8c35e22c3931c280966345bcaadda1c4f8b38e3e. Reason for revert: Reverting to fix unit_tests on chromeos bot. This is next in the chain. Ugh! Original change's description: > Add background tab colors and inactive images to the theme system. > > These will eventually be used for three purposes: > (1) To allow generating different background tab images from a common tint for > the inactive and incognito cases, thus allowing background tabs that blend > into the frame for custom themes. > (2) To allow autogenerating the background tab colors from the background tab > images, thus letting us autocalculate better tab text/separator/etc. colors. > (3) To allow theme authors to customize/override this behavior more precisely. > > Reasons (1) and (2) dictated the design here; reason (3) is basically a free > bonus. I thought about not making these resources overrideable by theme > authors, but there was some technical reason that turned out to not be easily > feasible, and there seems little reason to prevent it. > > This also allows us to read THEME_TAB_BACKGROUND_INCOGNITO on Mac (Views). > > This shouldn't result in a visual change on its own. > > Bug: 862664 > Change-Id: Icc57f78fd526d68b83c692fbac669a7371bb38ff > Reviewed-on: https://chromium-review.googlesource.com/1147869 > Commit-Queue: Peter Kasting <pkasting@chromium.org> > Reviewed-by: Mitsuru Oshima <oshima@chromium.org> > Reviewed-by: Evan Stade <estade@chromium.org> > Reviewed-by: Allen Bauer <kylixrd@chromium.org> > Reviewed-by: Bret Sepulveda <bsep@chromium.org> > Cr-Commit-Position: refs/heads/master@{#578790} TBR=pkasting@chromium.org,oshima@chromium.org,estade@chromium.org,bsep@chromium.org,kylixrd@chromium.org Change-Id: I6abe212768916890c11aec28aac5fc96891ac695 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 862664 Reviewed-on: https://chromium-review.googlesource.com/1153990 Reviewed-by: Scott Violet <sky@chromium.org> Commit-Queue: Scott Violet <sky@chromium.org> Cr-Commit-Position: refs/heads/master@{#578861} [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/app/theme/theme_resources.grd [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/themes/browser_theme_pack_unittest.cc [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/themes/theme_properties.cc [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/themes/theme_properties.h [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/themes/theme_service.cc [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/ui/views/frame/browser_non_client_frame_view.h [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/ui/views/tabs/browser_tab_strip_controller.h [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.h [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/ui/views/tabs/tab_controller.h [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/ui/views/tabs/tab_strip.cc [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/ui/views/tabs/tab_strip.h [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/ui/views/tabs/tab_strip_controller.h [modify] https://crrev.com/62a8750bd3812074a383ad3e10bf99d5ed49e93c/chrome/browser/ui/views/tabs/tab_unittest.cc
,
Jul 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/edad6258464443787aa84302360fc0b97cfdcabe commit edad6258464443787aa84302360fc0b97cfdcabe Author: Peter Kasting <pkasting@chromium.org> Date: Sat Jul 28 02:23:11 2018 Reland "Add background tab colors and inactive images to the theme system." This is a reland of 8c35e22c3931c280966345bcaadda1c4f8b38e3e Original change's description: > Add background tab colors and inactive images to the theme system. > > These will eventually be used for three purposes: > (1) To allow generating different background tab images from a common tint for > the inactive and incognito cases, thus allowing background tabs that blend > into the frame for custom themes. > (2) To allow autogenerating the background tab colors from the background tab > images, thus letting us autocalculate better tab text/separator/etc. colors. > (3) To allow theme authors to customize/override this behavior more precisely. > > Reasons (1) and (2) dictated the design here; reason (3) is basically a free > bonus. I thought about not making these resources overrideable by theme > authors, but there was some technical reason that turned out to not be easily > feasible, and there seems little reason to prevent it. > > This also allows us to read THEME_TAB_BACKGROUND_INCOGNITO on Mac (Views). > > This shouldn't result in a visual change on its own. > > Bug: 862664 > Change-Id: Icc57f78fd526d68b83c692fbac669a7371bb38ff > Reviewed-on: https://chromium-review.googlesource.com/1147869 > Commit-Queue: Peter Kasting <pkasting@chromium.org> > Reviewed-by: Mitsuru Oshima <oshima@chromium.org> > Reviewed-by: Evan Stade <estade@chromium.org> > Reviewed-by: Allen Bauer <kylixrd@chromium.org> > Reviewed-by: Bret Sepulveda <bsep@chromium.org> > Cr-Commit-Position: refs/heads/master@{#578790} Bug: 862664 Change-Id: I341bbce16853095f17b2608a7639bf0fd105c2cc TBR: estade, oshima, bsep, kylixrd Reviewed-on: https://chromium-review.googlesource.com/1154148 Reviewed-by: Peter Kasting <pkasting@chromium.org> Commit-Queue: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#578898} [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/app/theme/theme_resources.grd [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/themes/browser_theme_pack_unittest.cc [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/themes/theme_properties.cc [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/themes/theme_properties.h [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/themes/theme_service.cc [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/ui/views/frame/browser_non_client_frame_view.h [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/ui/views/tabs/browser_tab_strip_controller.h [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.h [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/ui/views/tabs/tab_controller.h [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/ui/views/tabs/tab_strip.cc [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/ui/views/tabs/tab_strip.h [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/ui/views/tabs/tab_strip_controller.h [modify] https://crrev.com/edad6258464443787aa84302360fc0b97cfdcabe/chrome/browser/ui/views/tabs/tab_unittest.cc
,
Jul 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a4248b92419b60d2e4319448bf9995c7f834e45c commit a4248b92419b60d2e4319448bf9995c7f834e45c Author: Peter Kasting <pkasting@chromium.org> Date: Sat Jul 28 19:28:03 2018 Reland "Compute colors for the frame and tabs from provided images." This is a reland of df84748a54c4ccd92e732c9168a2b3e8925695f5 Original change's description: > Compute colors for the frame and tabs from provided images. > > If theme authors explicitly provide values here, we'll use them; but if they > don't, but do provide images, set the colors to be the dominant colors of the > images, using an existing K-means algorithm (similar to what we use for > computing representative favicon colors etc.). > > This allows us to rely on those colors later when deciding what color to make > tab text, the new tab button, and similar tabstrip/window frame items. > > This also does a bit of cleanup to the theme pack code, e.g. moving a map used > in only one function into that function to make its provenance clearer. I can > try to split this apart into more CLs if desired. > > Bug: 862664 > Change-Id: I8c3f15893ad491a6fee8aa5f76ae162cecb2c6fd > Reviewed-on: https://chromium-review.googlesource.com/1152517 > Commit-Queue: Peter Kasting <pkasting@chromium.org> > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> > Reviewed-by: Evan Stade <estade@chromium.org> > Cr-Commit-Position: refs/heads/master@{#578823} Bug: 862664 Change-Id: Ic5d9479a144f07f48de5bf4e2a07d6b6b0ea8f72 TBR: asvitkine, estade Reviewed-on: https://chromium-review.googlesource.com/1154150 Reviewed-by: Peter Kasting <pkasting@chromium.org> Commit-Queue: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/heads/master@{#578932} [modify] https://crrev.com/a4248b92419b60d2e4319448bf9995c7f834e45c/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/a4248b92419b60d2e4319448bf9995c7f834e45c/chrome/browser/themes/browser_theme_pack.h [modify] https://crrev.com/a4248b92419b60d2e4319448bf9995c7f834e45c/chrome/browser/themes/browser_theme_pack_unittest.cc [modify] https://crrev.com/a4248b92419b60d2e4319448bf9995c7f834e45c/ui/gfx/color_analysis.cc [modify] https://crrev.com/a4248b92419b60d2e4319448bf9995c7f834e45c/ui/gfx/color_analysis.h
,
Jul 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5ae6d69be3abbac3a2d696120919d26644698162 commit 5ae6d69be3abbac3a2d696120919d26644698162 Author: Peter Kasting <pkasting@chromium.org> Date: Sat Jul 28 21:53:25 2018 Base the tab separator color on the tab background color. Bug: 862664 Change-Id: I05e936b9f165278d46f7b9aee4dc1fe8dd20624c Reviewed-on: https://chromium-review.googlesource.com/1139410 Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#578937} [modify] https://crrev.com/5ae6d69be3abbac3a2d696120919d26644698162/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
,
Jul 28
,
Jul 29
This bug requires manual review: There is .grd file changes and we are only 36 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 29
Could you pls update which CL you're requesting a merge to M69 for? Also how safe is the CL to merge? Thank you.
,
Jul 30
All CLs on this bug: comments 16, 17, 18, and 23 (the relands in comments 21 and 22 are identical). This stuff isn't as safe as I'd like, but our hands are tied: we need this theme work to make Refresh work well with custom themes in GM2. I'm comfortable merging it and addressing any issues that happen to appear before stable.
,
Jul 30
Thank you pkasting@. Approving merge to M69 branch 3497 for all CLs listed at #27 based on comment #27. Please merge before 3:00 PM PT, tomorrow, Monday so we can pick it up for next week Dev/Beta release. Thank you.
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4136b5199cb58f17b65fc97027584e4c082f334f commit 4136b5199cb58f17b65fc97027584e4c082f334f Author: Peter Kasting <pkasting@chromium.org> Date: Mon Jul 30 19:21:53 2018 Make HasCustomColor() match GetColor(). The latter has various transformations around when it queries the underlying the provider and with what constants. Use this behavior in HasCustomColor() too. This ensures callers get consistent behavior in these cases. Along the way this changes a multi-arm conditional to a helper function. That isn't necessary for this patch, but it will make it easy to land subsequent ones that add more such constants. Bug: 862664 Change-Id: I83295902f7c38767c3a051a40e5f195f8e78e4ef Reviewed-on: https://chromium-review.googlesource.com/1147601 Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#577703}(cherry picked from commit a91ffca64e95ec36000ee43296d5025e89da5244) Reviewed-on: https://chromium-review.googlesource.com/1155489 Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#229} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/4136b5199cb58f17b65fc97027584e4c082f334f/chrome/browser/themes/theme_service.cc [modify] https://crrev.com/4136b5199cb58f17b65fc97027584e4c082f334f/chrome/browser/themes/theme_service.h
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2acf05201ec92c2e8e9bedc66551aebb7dd11d07 commit 2acf05201ec92c2e8e9bedc66551aebb7dd11d07 Author: Peter Kasting <pkasting@chromium.org> Date: Mon Jul 30 20:39:35 2018 Add background tab colors and inactive images to the theme system. These will eventually be used for three purposes: (1) To allow generating different background tab images from a common tint for the inactive and incognito cases, thus allowing background tabs that blend into the frame for custom themes. (2) To allow autogenerating the background tab colors from the background tab images, thus letting us autocalculate better tab text/separator/etc. colors. (3) To allow theme authors to customize/override this behavior more precisely. Reasons (1) and (2) dictated the design here; reason (3) is basically a free bonus. I thought about not making these resources overrideable by theme authors, but there was some technical reason that turned out to not be easily feasible, and there seems little reason to prevent it. This also allows us to read THEME_TAB_BACKGROUND_INCOGNITO on Mac (Views). This shouldn't result in a visual change on its own. Bug: 862664 Change-Id: Icc57f78fd526d68b83c692fbac669a7371bb38ff Reviewed-on: https://chromium-review.googlesource.com/1147869 Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Mitsuru Oshima <oshima@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Reviewed-by: Allen Bauer <kylixrd@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#578790}(cherry picked from commit 8c35e22c3931c280966345bcaadda1c4f8b38e3e) Reviewed-on: https://chromium-review.googlesource.com/1155608 Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#241} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/app/theme/theme_resources.grd [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/themes/browser_theme_pack_unittest.cc [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/themes/theme_properties.cc [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/themes/theme_properties.h [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/themes/theme_service.cc [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/ui/views/frame/browser_non_client_frame_view.h [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/ui/views/tabs/browser_tab_strip_controller.h [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.cc [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/ui/views/tabs/fake_base_tab_strip_controller.h [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/ui/views/tabs/tab_controller.h [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/ui/views/tabs/tab_strip.cc [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/ui/views/tabs/tab_strip.h [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/ui/views/tabs/tab_strip_controller.h [modify] https://crrev.com/2acf05201ec92c2e8e9bedc66551aebb7dd11d07/chrome/browser/ui/views/tabs/tab_unittest.cc
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/db037b94b66afd8d94e65a2cab220764a22064d8 commit db037b94b66afd8d94e65a2cab220764a22064d8 Author: Peter Kasting <pkasting@chromium.org> Date: Mon Jul 30 20:41:20 2018 Compute colors for the frame and tabs from provided images. If theme authors explicitly provide values here, we'll use them; but if they don't, but do provide images, set the colors to be the dominant colors of the images, using an existing K-means algorithm (similar to what we use for computing representative favicon colors etc.). This allows us to rely on those colors later when deciding what color to make tab text, the new tab button, and similar tabstrip/window frame items. This also does a bit of cleanup to the theme pack code, e.g. moving a map used in only one function into that function to make its provenance clearer. I can try to split this apart into more CLs if desired. Bug: 862664 Change-Id: I8c3f15893ad491a6fee8aa5f76ae162cecb2c6fd Reviewed-on: https://chromium-review.googlesource.com/1152517 Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#578823}(cherry picked from commit df84748a54c4ccd92e732c9168a2b3e8925695f5) Reviewed-on: https://chromium-review.googlesource.com/1155609 Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#243} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/db037b94b66afd8d94e65a2cab220764a22064d8/chrome/browser/themes/browser_theme_pack.cc [modify] https://crrev.com/db037b94b66afd8d94e65a2cab220764a22064d8/chrome/browser/themes/browser_theme_pack.h [modify] https://crrev.com/db037b94b66afd8d94e65a2cab220764a22064d8/chrome/browser/themes/browser_theme_pack_unittest.cc [modify] https://crrev.com/db037b94b66afd8d94e65a2cab220764a22064d8/ui/gfx/color_analysis.cc [modify] https://crrev.com/db037b94b66afd8d94e65a2cab220764a22064d8/ui/gfx/color_analysis.h
,
Jul 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/47e0de6c8cb400f98eb46662fcfbb9e10f622643 commit 47e0de6c8cb400f98eb46662fcfbb9e10f622643 Author: Peter Kasting <pkasting@chromium.org> Date: Mon Jul 30 20:42:27 2018 Base the tab separator color on the tab background color. Bug: 862664 Change-Id: I05e936b9f165278d46f7b9aee4dc1fe8dd20624c Reviewed-on: https://chromium-review.googlesource.com/1139410 Commit-Queue: Peter Kasting <pkasting@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#578937}(cherry picked from commit 5ae6d69be3abbac3a2d696120919d26644698162) Reviewed-on: https://chromium-review.googlesource.com/1155495 Reviewed-by: Peter Kasting <pkasting@chromium.org> Cr-Commit-Position: refs/branch-heads/3497@{#245} Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753} [modify] https://crrev.com/47e0de6c8cb400f98eb46662fcfbb9e10f622643/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
,
Jul 30
,
Aug 14
+abdulsyed@ fyi, M69 merges taken for Proj-MdRefresh . |
|||||||||||||||||||||||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||||||||||||||||||||||
Comment 1 by robliao@chromium.org
, Jul 12