New issue
Advanced search Search tips

Issue 905009 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Refactor ChromeTypographyProvider::GetFont

Project Member Reported by newcomer@chromium.org, Nov 13

Issue description


1. Make ChromeTypographyProvider::GetFont default to DefaultTypographyProvider::GetFont.

2. Move handling of views::style enums used in ChromeTypographyProvider to the base class, DefaultTypographyProvider.


 
Labels: -Pri-3 M-72 Pri-1
Description: Show this description
Cc: tapted@chromium.org
Status: Assigned (was: Started)
TypographyProvider owners (msw, taped) are you ok with this change?

I see that DefaultTypographyProvider's purpose is to "provide values to match pre-Harmony constants for the given contexts so that old UI does not change".

To me, this means we probably should not move the views::style enums referenced in ChromeTypographyProvider to DefaultTypographyProvider. This could modify the behavior of older UI. WDYT?
Cc: bsep@chromium.org pkasting@chromium.org
Components: Internals>Views
Labels: -Pri-1 Proj-MdRefresh Proj-MaterialDesign-NativeUI Pri-2
I'd think Views *should* use Harmony values, I'm not sure why we'd want anything to look "pre-Harmony" at this point.
It would help to audit the UI surfaces affected and the specific appearance changes that this would cause.
pkasting@, bsep@, any thoughts or ideas who to ping about the intent here? This refactoring is not P1.
See this CL comment thread for a bit more context:
  https://crrev.com/c/1329672/1/chrome/browser/ui/views/chrome_typography_provider.cc#136
Nothing should look pre-Harmony.  However, in the past, sky was insistent that concepts only used in chrome/ should be handled by providers defined in chrome/ and not handled by code in ui/views/.  So I would not proceed with the larger refactoring here without sky's approval.
Cc: sky@chromium.org
Adding sky@. The contexts handled explicitly in ChromeTypographyProvider::GetFont look agnostic to Chrome, IMO:
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_typography_provider.cc?rcl=e0f977777fecfecda53b05e5dcc50f9281204d66&l=114

Even CONTEXT_HEADLINE_OVERSIZED itself seems generic enough for Views (now in ApplyAshFontStyles):
https://cs.chromium.org/chromium/src/ash/public/cpp/ash_typography.cc?rcl=e349a45e258782e8e00713b0690f38d6001fc650&l=23

I'd also imagine that CONTEXT_WINDOWS10_NATIVE is not Chrome-specific (now in ApplyCommonFontStyles):
https://cs.chromium.org/chromium/src/chrome/browser/ui/views/chrome_typography.cc?rcl=e349a45e258782e8e00713b0690f38d6001fc650&l=82

Other things like launcher, toolbar, omnibox, definitely seem more chrome-specific.

Sign in to add a comment