Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1516233003:
Incognito NTP style tweaks (Closed)
|
Created:
5 years ago by edwardjung Modified:
4 years, 11 months ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, arv+watch_chromium.org, pedrosimonetti+watch_chromium.org, Dan Beam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIncognito NTP style tweaks
BUG=512984
Committed: https://crrev.com/b7dac1a6a7997c1c7bb440d5eb5a4aa6ee38cf8b
Cr-Commit-Position: refs/heads/master@{#369736}
Patch Set 1 #Patch Set 2 : Add Roboto Light #
Total comments: 2
Patch Set 3 : Address nits. Fix button padding. #
Total comments: 10
Patch Set 4 : Add minimum content width #
Total comments: 6
Patch Set 5 : Accommodate minimum Windows width #
Total comments: 2
Patch Set 6 : Revert the use of Roboto font #Patch Set 7 : Reinstate Roboto and add text_defaults_md.css #
Total comments: 12
Patch Set 8 : Include Roboto font via @import #Patch Set 9 : Update Roboto font with full charset version #Messages
Total messages: 42 (13 generated)
edwardjung@chromium.org changed reviewers: + estade@chromium.org, mathp@google.com
estade, please take a look at these minor tweaks. mathp, I've added the woff2 binary for Roboto light taken from Google Web Fonts. If it's some other version let me know.
mathp@chromium.org changed reviewers: + mathp@chromium.org
estade@ is owner of everything here, but the roboto change lgtm with nits https://codereview.chromium.org/1516233003/diff/20001/chrome/app/generated_re... File chrome/app/generated_resources.grd (left): https://codereview.chromium.org/1516233003/diff/20001/chrome/app/generated_re... chrome/app/generated_resources.grd:10570: desc="OTR window warning message"> Is this supposed to follow another sentence? If so, you should mention it in the description, so that the translators know how to best translate this. https://codereview.chromium.org/1516233003/diff/20001/ui/webui/resources/webu... File ui/webui/resources/webui_resources.grd (left): https://codereview.chromium.org/1516233003/diff/20001/ui/webui/resources/webu... ui/webui/resources/webui_resources.grd:20: Roboto-Medium is not used on Android. --> update comment to mention Light
https://codereview.chromium.org/1516233003/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1516233003/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:10571: + However, you aren’t invisible. Going incognito doesn’t hide your browsing from your employer, your internet service provider, or the websites you visit. it's unfortunate this is duplicated with the android string (the android version seems to mash both paragraphs together) https://codereview.chromium.org/1516233003/diff/40001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (left): https://codereview.chromium.org/1516233003/diff/40001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:11: margin-bottom: 1.33em; why change this https://codereview.chromium.org/1516233003/diff/40001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:35: width: 420px; I don't think you should change this. - Horizontal scrollbars when narrow. that's intentional. It looks dumb/is not functional when you make the browser super narrow and the text becomes a giant column.
Description was changed from ========== Incognito NTP style tweaks BUG=512984 ========== to ========== Incognito NTP style tweaks BUG=512984 ==========
mathp@chromium.org changed reviewers: - mathp@google.com
Thanks estade. https://codereview.chromium.org/1516233003/diff/40001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/1516233003/diff/40001/chrome/app/generated_re... chrome/app/generated_resources.grd:10571: + However, you aren’t invisible. Going incognito doesn’t hide your browsing from your employer, your internet service provider, or the websites you visit. On 2015/12/14 20:27:22, Evan Stade wrote: > it's unfortunate this is duplicated with the android string (the android version > seems to mash both paragraphs together) Yeah, it would be nice to reduce this duplication. I can change the desktop to use a single message string but we'll need to add line breaks tags into the string. I'm not sure if the Android page is rendered as a web UI since the string doesn't have HTML tags to create the paragraph break. https://codereview.chromium.org/1516233003/diff/40001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (left): https://codereview.chromium.org/1516233003/diff/40001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:11: margin-bottom: 1.33em; On 2015/12/14 20:27:22, Evan Stade wrote: > why change this It rounds up the margin to 32px rather than 31.920px. https://codereview.chromium.org/1516233003/diff/40001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:35: width: 420px; > that's intentional. It looks dumb/is not functional when you make the browser > super narrow and the text becomes a giant column. Agree that when it's really narrow it doesn't look good. Users would have to be in Dev Tools to resize to such a narrow column. I've added a minimum width and adjusted the margin when it's narrow so at least most users won't get the horizontal scrollbar.
https://codereview.chromium.org/1516233003/diff/40001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (left): https://codereview.chromium.org/1516233003/diff/40001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:11: margin-bottom: 1.33em; On 2015/12/15 11:30:39, edwardjung wrote: > On 2015/12/14 20:27:22, Evan Stade wrote: > > why change this > > It rounds up the margin to 32px rather than 31.920px. why is 0.08px important? https://codereview.chromium.org/1516233003/diff/40001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:35: width: 420px; On 2015/12/15 11:30:39, edwardjung wrote: > > that's intentional. It looks dumb/is not functional when you make the browser > > super narrow and the text becomes a giant column. > > Agree that when it's really narrow it doesn't look good. Users would have to be > in Dev Tools to resize to such a narrow column. that is up to the window manager you're using and it's not always true. https://codereview.chromium.org/1516233003/diff/60001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): https://codereview.chromium.org/1516233003/diff/60001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:34: min-width: 300px; I still don't think this needs to change. The current sizing is intentional and works fine. A horizontal scrollbar for a very narrow window is quite reasonable. I prefer simpler CSS over more complicated CSS because it's very easy for some CSS to have no effect after you make some change and for it to not be apparent what situation, if any, a particular line of CSS is relevant to. https://codereview.chromium.org/1516233003/diff/60001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_tab.css (right): https://codereview.chromium.org/1516233003/diff/60001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:12: font-family: Roboto; shouldn't the guest tab get this too? https://codereview.chromium.org/1516233003/diff/60001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:28: display: inline-block; What is this accomplishing? Really this link should be a polymer button. I think you should leave it alone for now, and make it a polymer button later (or now) if you have the time.
https://codereview.chromium.org/1516233003/diff/40001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (left): https://codereview.chromium.org/1516233003/diff/40001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:11: margin-bottom: 1.33em; On 2015/12/15 19:25:14, Evan Stade wrote: > On 2015/12/15 11:30:39, edwardjung wrote: > > On 2015/12/14 20:27:22, Evan Stade wrote: > > > why change this > > > > It rounds up the margin to 32px rather than 31.920px. > > why is 0.08px important? In this instance I guess it doesn't matter as it's probably rounded up anyway. I was just trying to be accurate to the spec. https://codereview.chromium.org/1516233003/diff/40001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:35: width: 420px; > that is up to the window manager you're using and it's not always true. Understood they all vary and did some tweaks to make it work better for Windows which has the narrowest screen width at 270px. The line length is still very readable and better than being cut off. https://codereview.chromium.org/1516233003/diff/60001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): https://codereview.chromium.org/1516233003/diff/60001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:34: min-width: 300px; On 2015/12/15 19:25:14, Evan Stade wrote: > I still don't think this needs to change. The current sizing is intentional and > works fine. A horizontal scrollbar for a very narrow window is quite reasonable. > I prefer simpler CSS over more complicated CSS because it's very easy for some > CSS to have no effect after you make some change and for it to not be apparent > what situation, if any, a particular line of CSS is relevant to. I disagree, since most horizontal scrolling on web pages is poor UX. It just feels unpolished and like we don't know anything about responsive design techniques. The code isn't too complicated (I can add comments in the relevant media query section if that helps for future maintenance). I'll defer to bettes@ and sgabriel@ is whether they are okay with horizontal scrolling in the bug. Agreed that on very narrow widths (sub 270px) the single word on a line column is silly but the min-width caters for this. https://codereview.chromium.org/1516233003/diff/60001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_tab.css (right): https://codereview.chromium.org/1516233003/diff/60001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:12: font-family: Roboto; On 2015/12/15 19:25:14, Evan Stade wrote: > shouldn't the guest tab get this too? Good point. https://codereview.chromium.org/1516233003/diff/60001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_tab.css:28: display: inline-block; On 2015/12/15 19:25:14, Evan Stade wrote: > What is this accomplishing? Really this link should be a polymer button. I think > you should leave it alone for now, and make it a polymer button later (or now) > if you have the time. Links are by default display: inline, and they inline elements don't respond to the padding adjustments as specified in the spec. Yes, this should probably be a polymer button but I'm away from Friday and won't get round to doing that implementation until the new year.
For reference, I've added implementation screenshots for the Mac (widest min width at 400px) and Windows (narrowest 270px) into the bug: https://code.google.com/p/chromium/issues/detail?id=512984#c11 https://code.google.com/p/chromium/issues/detail?id=512984#c12
Added dbeam for comment regarding the responsive design implementation. estade wants to content area to be fixed which introduces horizontal scrolling. My implementation here uses a min-width to the narrowest (Windows) width so that the text is readable without scrolling on all platforms.
@estade any comments on my updates? See the implementation screenshots.
padding/scrolling change looks ok https://codereview.chromium.org/1516233003/diff/80001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): https://codereview.chromium.org/1516233003/diff/80001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:32: font-family: Roboto; I realize bettes asked for Roboto, but I don't think this is the right time or place to do it. He said: "1. Typography: Switch from the native OS font to Roboto for brand consistency with all chrome webUI" But most other WebUI is still not using Roboto, so this is actually introducing an inconsistency. That's why I don't think it's the right /time/. However, if we do decide we want spiffy new fonts over consistency and don't mind a piecemeal rollout, this is still not the right place. The way the font face is specified is in ui/webui/resources/css/text_defaults.css which is filled in differently for every platform. That means you should be changing this function[1] for the appropriate page(s) you want to change to roboto. [1] https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/webui/web_...
On 2016/01/09 00:31:18, Evan Stade wrote: > padding/scrolling change looks ok > > https://codereview.chromium.org/1516233003/diff/80001/chrome/browser/resource... > File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): > > https://codereview.chromium.org/1516233003/diff/80001/chrome/browser/resource... > chrome/browser/resources/ntp4/incognito_and_guest_tab.css:32: font-family: > Roboto; > I realize bettes asked for Roboto, but I don't think this is the right time or > place to do it. He said: > > "1. Typography: Switch from the native OS font to Roboto for brand consistency > with all chrome webUI" > > But most other WebUI is still not using Roboto, so this is actually introducing > an inconsistency. That's why I don't think it's the right /time/. Sure, that's reasonable. We can wait until the settings are done. Is it worth keeping in Roboto Light as part of the CL? I assume that other Materialised designs will use Roboto Light. > > However, if we do decide we want spiffy new fonts over consistency and don't > mind a piecemeal rollout, this is still not the right place. The way the font > face is specified is in ui/webui/resources/css/text_defaults.css which is filled > in differently for every platform. That means you should be changing this > function[1] for the appropriate page(s) you want to change to roboto. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/webui/web_... Thanks for the tip. Will bear this in mind for when the time comes.
On 2016/01/11 15:43:06, edwardjung wrote: > On 2016/01/09 00:31:18, Evan Stade wrote: > > padding/scrolling change looks ok > > > > > https://codereview.chromium.org/1516233003/diff/80001/chrome/browser/resource... > > File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): > > > > > https://codereview.chromium.org/1516233003/diff/80001/chrome/browser/resource... > > chrome/browser/resources/ntp4/incognito_and_guest_tab.css:32: font-family: > > Roboto; > > I realize bettes asked for Roboto, but I don't think this is the right time or > > place to do it. He said: > > > > "1. Typography: Switch from the native OS font to Roboto for brand consistency > > with all chrome webUI" > > > > But most other WebUI is still not using Roboto, so this is actually > introducing > > an inconsistency. That's why I don't think it's the right /time/. > > Sure, that's reasonable. We can wait until the settings are done. > > Is it worth keeping in Roboto Light as part of the CL? I assume that other > Materialised designs will use Roboto Light. > > > > > However, if we do decide we want spiffy new fonts over consistency and don't > > mind a piecemeal rollout, this is still not the right place. The way the font > > face is specified is in ui/webui/resources/css/text_defaults.css which is > filled > > in differently for every platform. That means you should be changing this > > function[1] for the appropriate page(s) you want to change to roboto. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/webui/web_... > > Thanks for the tip. Will bear this in mind for when the time comes. yea, you can keep the roboto light change and ping https://code.google.com/p/chromium/issues/detail?id=569137 when this lands. Assuming you tested this in both incognito and guest new tabs, LGTM
On 2016/01/11 17:50:29, Evan Stade wrote: > On 2016/01/11 15:43:06, edwardjung wrote: > > On 2016/01/09 00:31:18, Evan Stade wrote: > > > padding/scrolling change looks ok > > > > > > > > > https://codereview.chromium.org/1516233003/diff/80001/chrome/browser/resource... > > > File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): > > > > > > > > > https://codereview.chromium.org/1516233003/diff/80001/chrome/browser/resource... > > > chrome/browser/resources/ntp4/incognito_and_guest_tab.css:32: font-family: > > > Roboto; > > > I realize bettes asked for Roboto, but I don't think this is the right time > or > > > place to do it. He said: > > > > > > "1. Typography: Switch from the native OS font to Roboto for brand > consistency > > > with all chrome webUI" > > > > > > But most other WebUI is still not using Roboto, so this is actually > > introducing > > > an inconsistency. That's why I don't think it's the right /time/. > > > > Sure, that's reasonable. We can wait until the settings are done. > > > > Is it worth keeping in Roboto Light as part of the CL? I assume that other > > Materialised designs will use Roboto Light. > > > > > > > > However, if we do decide we want spiffy new fonts over consistency and don't > > > mind a piecemeal rollout, this is still not the right place. The way the > font > > > face is specified is in ui/webui/resources/css/text_defaults.css which is > > filled > > > in differently for every platform. That means you should be changing this > > > function[1] for the appropriate page(s) you want to change to roboto. > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/webui/web_... > > > > Thanks for the tip. Will bear this in mind for when the time comes. > > yea, you can keep the roboto light change and ping > https://code.google.com/p/chromium/issues/detail?id=569137 when this lands. > > Assuming you tested this in both incognito and guest new tabs, LGTM Yep both, tested on Mac and Linux, thanks.
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1516233003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1516233003/100001
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1516233003/diff/80001/chrome/browser/resource... File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): https://codereview.chromium.org/1516233003/diff/80001/chrome/browser/resource... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:32: font-family: Roboto; On 2016/01/09 00:31:18, Evan Stade wrote: > I realize bettes asked for Roboto, but I don't think this is the right time or > place to do it. He said: > > "1. Typography: Switch from the native OS font to Roboto for brand consistency > with all chrome webUI" > > But most other WebUI is still not using Roboto, so this is actually introducing > an inconsistency. That's why I don't think it's the right /time/. > > However, if we do decide we want spiffy new fonts over consistency and don't > mind a piecemeal rollout, this is still not the right place. The way the font > face is specified is in ui/webui/resources/css/text_defaults.css which is filled > in differently for every platform. That means you should be changing this > function[1] for the appropriate page(s) you want to change to roboto. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/webui/web_... the Material Design webui fonts wont differ per-platform, so this wouldn't need to be done dynamically (as it previously has been). you can just use roboto.css and "Roboto" as the font-family on new MD WebUI pages. varying weights changes from Light/Medium. i don't currently see a reason for fallback fonts. we could create a new text_defaults_md.css that hardcodes to Roboto (but leaves directionality and other things the same)?
On 2016/01/11 20:11:56, Dan Beam wrote: > https://codereview.chromium.org/1516233003/diff/80001/chrome/browser/resource... > File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): > > https://codereview.chromium.org/1516233003/diff/80001/chrome/browser/resource... > chrome/browser/resources/ntp4/incognito_and_guest_tab.css:32: font-family: > Roboto; > On 2016/01/09 00:31:18, Evan Stade wrote: > > I realize bettes asked for Roboto, but I don't think this is the right time or > > place to do it. He said: > > > > "1. Typography: Switch from the native OS font to Roboto for brand consistency > > with all chrome webUI" > > > > But most other WebUI is still not using Roboto, so this is actually > introducing > > an inconsistency. That's why I don't think it's the right /time/. > > > > However, if we do decide we want spiffy new fonts over consistency and don't > > mind a piecemeal rollout, this is still not the right place. The way the font > > face is specified is in ui/webui/resources/css/text_defaults.css which is > filled > > in differently for every platform. That means you should be changing this > > function[1] for the appropriate page(s) you want to change to roboto. > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/base/webui/web_... > > the Material Design webui fonts wont differ per-platform, so this wouldn't need > to be done dynamically (as it previously has been). > > you can just use roboto.css and "Roboto" as the font-family on new MD WebUI > pages. varying weights changes from Light/Medium. i don't currently see a > reason for fallback fonts. > > we could create a new text_defaults_md.css that hardcodes to Roboto (but leaves > directionality and other things the same)? sgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/01/11 21:30:19, commit-bot: I haz the power wrote: > Dry run: This issue passed the CQ dry run.
> the Material Design webui fonts wont differ per-platform, so this wouldn't need > to be done dynamically (as it previously has been). > > you can just use roboto.css and "Roboto" as the font-family on new MD WebUI > pages. varying weights changes from Light/Medium. > > we could create a new text_defaults_md.css that hardcodes to Roboto (but leaves > directionality and other things the same)? Thanks dbeam. I've reinstated Roboto to the pages and created text_defaults_md.css. Not sure if my implementation is in line with your thinking. > i don't currently see a reason for fallback fonts. I think we should keep fallback fonts. I don't think Roboto fully supports all non-latin character sets. In the spec Noto is specified for CJK for instance. https://www.google.com/design/spec/style/typography.html#typography-typeface So accordingly I've left the back up fonts in text_defaults_md
https://codereview.chromium.org/1516233003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): https://codereview.chromium.org/1516233003/diff/120001/chrome/browser/resourc... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:32: font-family: Roboto; do you need this if you're pulling in text_defaults_md.css? or if that's not working, can this be `font-family: inherit` instead? https://codereview.chromium.org/1516233003/diff/120001/ui/base/webui/web_ui_u... File ui/base/webui/web_ui_util.cc (right): https://codereview.chromium.org/1516233003/diff/120001/ui/base/webui/web_ui_u... ui/base/webui/web_ui_util.cc:125: std::string GetWebUiCssTextDefaults(std::string css_template) { const-ref https://codereview.chromium.org/1516233003/diff/120001/ui/base/webui/web_ui_u... File ui/base/webui/web_ui_util.h (right): https://codereview.chromium.org/1516233003/diff/120001/ui/base/webui/web_ui_u... ui/base/webui/web_ui_util.h:61: UI_BASE_EXPORT std::string GetWebUiCssTextDefaultsMD(); MD or Md? ugh https://codereview.chromium.org/1516233003/diff/120001/ui/webui/resources/css... File ui/webui/resources/css/roboto.css (right): https://codereview.chromium.org/1516233003/diff/120001/ui/webui/resources/css... ui/webui/resources/css/roboto.css:10: url(chrome://resources/roboto/roboto-light.woff2) format('woff2'); this will probably piss off some bots (it's gonna make chrome bigger) https://codereview.chromium.org/1516233003/diff/120001/ui/webui/resources/css... File ui/webui/resources/css/text_defaults_md.css (right): https://codereview.chromium.org/1516233003/diff/120001/ui/webui/resources/css... ui/webui/resources/css/text_defaults_md.css:17: can we make this file pull in roboto.css automagically, i.e.: @import url(roboto.css); ?
https://codereview.chromium.org/1516233003/diff/120001/ui/base/webui/web_ui_u... File ui/base/webui/web_ui_util.h (right): https://codereview.chromium.org/1516233003/diff/120001/ui/base/webui/web_ui_u... ui/base/webui/web_ui_util.h:61: UI_BASE_EXPORT std::string GetWebUiCssTextDefaultsMD(); On 2016/01/12 23:05:52, Dan Beam wrote: > MD or Md? ugh style guide clearly states Md
https://codereview.chromium.org/1516233003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): https://codereview.chromium.org/1516233003/diff/120001/chrome/browser/resourc... chrome/browser/resources/ntp4/incognito_and_guest_tab.css:32: font-family: Roboto; On 2016/01/12 23:05:52, Dan Beam wrote: > do you need this if you're pulling in text_defaults_md.css? or if that's not > working, can this be `font-family: inherit` instead? Removed, it's working without. https://codereview.chromium.org/1516233003/diff/120001/ui/base/webui/web_ui_u... File ui/base/webui/web_ui_util.cc (right): https://codereview.chromium.org/1516233003/diff/120001/ui/base/webui/web_ui_u... ui/base/webui/web_ui_util.cc:125: std::string GetWebUiCssTextDefaults(std::string css_template) { On 2016/01/12 23:05:52, Dan Beam wrote: > const-ref Done. https://codereview.chromium.org/1516233003/diff/120001/ui/base/webui/web_ui_u... File ui/base/webui/web_ui_util.h (right): https://codereview.chromium.org/1516233003/diff/120001/ui/base/webui/web_ui_u... ui/base/webui/web_ui_util.h:61: UI_BASE_EXPORT std::string GetWebUiCssTextDefaultsMD(); On 2016/01/13 04:14:53, Evan Stade wrote: > On 2016/01/12 23:05:52, Dan Beam wrote: > > MD or Md? ugh > > style guide clearly states Md Done. https://codereview.chromium.org/1516233003/diff/120001/ui/webui/resources/css... File ui/webui/resources/css/roboto.css (right): https://codereview.chromium.org/1516233003/diff/120001/ui/webui/resources/css... ui/webui/resources/css/roboto.css:10: url(chrome://resources/roboto/roboto-light.woff2) format('woff2'); On 2016/01/12 23:05:52, Dan Beam wrote: > this will probably piss off some bots (it's gonna make chrome bigger) What's the procedure for getting this through? Anything I can do before committing. https://codereview.chromium.org/1516233003/diff/120001/ui/webui/resources/css... File ui/webui/resources/css/text_defaults_md.css (right): https://codereview.chromium.org/1516233003/diff/120001/ui/webui/resources/css... ui/webui/resources/css/text_defaults_md.css:17: On 2016/01/12 23:05:52, Dan Beam wrote: > can we make this file pull in roboto.css automagically, i.e.: > > @import url(roboto.css); > > ? Yep, that works.
https://codereview.chromium.org/1516233003/diff/120001/ui/webui/resources/css... File ui/webui/resources/css/roboto.css (right): https://codereview.chromium.org/1516233003/diff/120001/ui/webui/resources/css... ui/webui/resources/css/roboto.css:10: url(chrome://resources/roboto/roboto-light.woff2) format('woff2'); On 2016/01/13 21:24:07, edwardjung wrote: > On 2016/01/12 23:05:52, Dan Beam wrote: > > this will probably piss off some bots (it's gonna make chrome bigger) > > What's the procedure for getting this through? Anything I can do before > committing. not sure. maybe it wont trip the bots? but you could pre-check with perf sheriffs or TPMs regarding a sizes increase, but they're gonna argue their hardest to convince you it's not necessary. you'd need ammo to convince them it is.
On 2016/01/13 21:51:41, Dan Beam wrote: > https://codereview.chromium.org/1516233003/diff/120001/ui/webui/resources/css... > File ui/webui/resources/css/roboto.css (right): > > https://codereview.chromium.org/1516233003/diff/120001/ui/webui/resources/css... > ui/webui/resources/css/roboto.css:10: > url(chrome://resources/roboto/roboto-light.woff2) format('woff2'); > On 2016/01/13 21:24:07, edwardjung wrote: > > On 2016/01/12 23:05:52, Dan Beam wrote: > > > this will probably piss off some bots (it's gonna make chrome bigger) > > > > What's the procedure for getting this through? Anything I can do before > > committing. > > not sure. maybe it wont trip the bots? but you could pre-check with perf > sheriffs or TPMs regarding a sizes increase, but they're gonna argue their > hardest to convince you it's not necessary. you'd need ammo to convince them it > is. warn the sheriffs and cross your fingers
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1516233003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1516233003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by edwardjung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/1516233003/#ps160001 (title: "Update Roboto font with full charset version")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1516233003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1516233003/160001
Message was sent while issue was closed.
Description was changed from ========== Incognito NTP style tweaks BUG=512984 ========== to ========== Incognito NTP style tweaks BUG=512984 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Incognito NTP style tweaks BUG=512984 ========== to ========== Incognito NTP style tweaks BUG=512984 Committed: https://crrev.com/b7dac1a6a7997c1c7bb440d5eb5a4aa6ee38cf8b Cr-Commit-Position: refs/heads/master@{#369736} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/b7dac1a6a7997c1c7bb440d5eb5a4aa6ee38cf8b Cr-Commit-Position: refs/heads/master@{#369736} |
