chrome/browser/ui/webui/ntp/ntp_resource_cache.cc - Issue 1440113002: [MD] Change appearance of incognito NTP to MD spec.

Unified Diff: chrome/browser/ui/webui/ntp/ntp_resource_cache.cc

Issue 1440113002: [MD] Change appearance of incognito NTP to MD spec. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: guest Created 5 years, 1 month ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« no previous file with comments | « chrome/browser/ui/webui/ntp/ntp_resource_cache.h ('k') | chrome/browser/ui/webui/theme_source.cc » ('j') | chrome/browser/ui/webui/theme_source.cc » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/ui/webui/ntp/ntp_resource_cache.cc

diff --git a/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc b/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc

index 5215c672ff6cbcc2ccec700197f1f28d3651c14b..253a3254c7c041e9a5defff3cfe7941c426d1b2f 100644

--- a/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc

+++ b/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc

@@ -261,19 +261,17 @@ base::RefCountedMemory* NTPResourceCache::GetNewTabHTML(WindowType win_type) {

base::RefCountedMemory* NTPResourceCache::GetNewTabCSS(WindowType win_type) {

DCHECK_CURRENTLY_ON(BrowserThread::UI);

- if (win_type == GUEST) {

- if (!new_tab_guest_css_.get())

- CreateNewTabGuestCSS();

- return new_tab_guest_css_.get();

- } else if (win_type == INCOGNITO) {

+ DCHECK_NE(win_type, GUEST);

+ if (win_type == INCOGNITO) {

if (!new_tab_incognito_css_.get())

CreateNewTabIncognitoCSS();

return new_tab_incognito_css_.get();

- } else {

- if (!new_tab_css_.get())

- CreateNewTabCSS();

- return new_tab_css_.get();

}

+ if (!new_tab_css_.get())

Dan Beam 2015/11/13 01:32:22 nit: .get() isn't necessary here

Evan Stade 2015/11/13 01:39:00 Acknowledged (will fix).

Evan Stade 2015/11/19 23:05:24 Done.

+ CreateNewTabCSS();

+ return new_tab_css_.get();

}

void NTPResourceCache::Observe(int type,

@@ -539,7 +537,9 @@ void NTPResourceCache::CreateNewTabIncognitoCSS() {

// Get our theme colors

SkColor color_background =

- GetThemeColor(tp, ThemeProperties::COLOR_NTP_BACKGROUND);

+ tp->HasCustomImage(IDR_THEME_NTP_BACKGROUND)

+ ? GetThemeColor(tp, ThemeProperties::COLOR_NTP_BACKGROUND)

+ : SkColorSetRGB(0x32, 0x32, 0x32);

// Generate the replacements.

std::map<base::StringPiece, std::string> substitutions;

@@ -553,6 +553,10 @@ void NTPResourceCache::CreateNewTabIncognitoCSS() {

substitutions["backgroundBarDetached"] = GetNewTabBackgroundCSS(tp, false);

substitutions["backgroundBarAttached"] = GetNewTabBackgroundCSS(tp, true);

substitutions["backgroundTiling"] = GetNewTabBackgroundTilingCSS(tp);

+ substitutions["hasCustomBackground"] =

+ !tp->HasCustomImage(IDR_THEME_NTP_BACKGROUND)

+ ? ".doesNotHaveCustomBackground"

Dan Beam 2015/11/13 01:32:22 i get that this is simple, but I think it is very

Evan Stade 2015/11/13 01:39:00 That sounds nightmarishly difficult because only t

Dan Beam 2015/11/16 19:57:25 can't you just add a hasCustomBackground boolean p

Evan Stade 2015/11/16 20:10:08 no. That would work for any NTPs you open after a

Dan Beam 2015/11/17 00:18:59 i'm saying pass a hasCustomBackground boolean to j

i'm saying pass a hasCustomBackground boolean to js, in the js do: document.classList.toggle('has-custom-background', hasCustomBackground); then reload the theme stylesheet. that would update the running HTML page.

Dan Beam 2015/11/17 00:26:15 document.body.**

On 2015/11/17 00:18:59, Dan Beam wrote: > On 2015/11/16 20:10:08, Evan Stade wrote: > > On 2015/11/16 19:57:25, Dan Beam wrote: > > > On 2015/11/13 01:39:00, Evan Stade wrote: > > > > On 2015/11/13 01:32:22, Dan Beam wrote: > > > > > i get that this is simple, but I think it is very different to how you > > > > typically > > > > > see classes applied in CSS > > > > > > > > > > could we invert the logic and add: > > > > > > > > > > <body class="${hasCustomBackground}"> > > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > in the HTML and run ui::ReplaceTemplateExpressions() on it? fwiw: we'd > > like > > > > to > > > > > eventually move all > > > > > > > > > > <div i18n-content="blah"></div> > > > > > > > > > > to > > > > > > > > > > <div>${blah}</div> > > > > > > > > > > to reduce flicker, remove the need to do i18nTemplate.process() when the > > > page > > > > is > > > > > starting, and in theory for better security (would remove some uses of > > > > > innerHTML). this is a very simple page and a nice place to start. > > > > > > > > That sounds nightmarishly difficult because only the theme css changes > when > > > the > > > > theme changes, and refreshing HTML too would be a huge headache, so I'd > > really > > > > like to do this in the css, in this function. > > > > > > can't you just add a hasCustomBackground boolean parameter to this and also > > null > > > out the cached HTML? > > > > no. That would work for any NTPs you open after a theme change, but it would > not > > update the NTPs that are already open. > > i'm saying pass a hasCustomBackground boolean to js, in the js do: > > document.classList.toggle('has-custom-background', hasCustomBackground); > > then reload the theme stylesheet. that would update the running HTML page. > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > > > > > > I agree that it's oddball and backwards from what you'd expect. I think > this > > > > whole file is a bit crooked so it's tricky to build something stable on > top > > of > > > > it. > > > > > > i don't think something being oddly organized is a reason to make it more > odd > > >

document.body.**

Evan Stade 2015/11/19 23:05:24 Done.

On 2015/11/17 00:26:15, Dan Beam wrote: > On 2015/11/17 00:18:59, Dan Beam wrote: > > On 2015/11/16 20:10:08, Evan Stade wrote: > > > On 2015/11/16 19:57:25, Dan Beam wrote: > > > > On 2015/11/13 01:39:00, Evan Stade wrote: > > > > > On 2015/11/13 01:32:22, Dan Beam wrote: > > > > > > i get that this is simple, but I think it is very different to how you > > > > > typically > > > > > > see classes applied in CSS > > > > > > > > > > > > could we invert the logic and add: > > > > > > > > > > > > <body class="${hasCustomBackground}"> > > > > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > > > > > in the HTML and run ui::ReplaceTemplateExpressions() on it? fwiw: > we'd > > > like > > > > > to > > > > > > eventually move all > > > > > > > > > > > > <div i18n-content="blah"></div> > > > > > > > > > > > > to > > > > > > > > > > > > <div>${blah}</div> > > > > > > > > > > > > to reduce flicker, remove the need to do i18nTemplate.process() when > the > > > > page > > > > > is > > > > > > starting, and in theory for better security (would remove some uses of > > > > > > innerHTML). this is a very simple page and a nice place to start. > > > > > > > > > > That sounds nightmarishly difficult because only the theme css changes > > when > > > > the > > > > > theme changes, and refreshing HTML too would be a huge headache, so I'd > > > really > > > > > like to do this in the css, in this function. > > > > > > > > can't you just add a hasCustomBackground boolean parameter to this and > also > > > null > > > > out the cached HTML? > > > > > > no. That would work for any NTPs you open after a theme change, but it would > > not > > > update the NTPs that are already open. > > > > i'm saying pass a hasCustomBackground boolean to js, in the js do: > > > > document.classList.toggle('has-custom-background', hasCustomBackground); > > > > then reload the theme stylesheet. that would update the running HTML page. > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > > > > > > > > > > > I agree that it's oddball and backwards from what you'd expect. I think > > this > > > > > whole file is a bit crooked so it's tricky to build something stable on > > top > > > of > > > > > it. > > > > > > > > i don't think something being oddly organized is a reason to make it more > > odd > > > > > > > document.body.**

Done.

+ : std::string();

// Get our template.

static const base::StringPiece new_tab_theme_css(

@@ -566,39 +570,6 @@ void NTPResourceCache::CreateNewTabIncognitoCSS() {

new_tab_incognito_css_ = base::RefCountedString::TakeString(&full_css);

}

-void NTPResourceCache::CreateNewTabGuestCSS() {

- ui::ThemeProvider* tp = ThemeServiceFactory::GetForProfile(profile_);

- DCHECK(tp);

- // Get our theme colors

- SkColor color_background =

- GetThemeColor(tp, ThemeProperties::COLOR_NTP_BACKGROUND);

- // Generate the replacements.

- std::map<base::StringPiece, std::string> substitutions;

- // Cache-buster for background.

- substitutions["themeId"] =

- profile_->GetPrefs()->GetString(prefs::kCurrentThemeID);

- // Colors.

- substitutions["colorBackground"] = SkColorToRGBAString(color_background);

- substitutions["backgroundBarDetached"] = GetNewTabBackgroundCSS(tp, false);

- substitutions["backgroundBarAttached"] = GetNewTabBackgroundCSS(tp, true);

- substitutions["backgroundTiling"] = GetNewTabBackgroundTilingCSS(tp);

- // Get our template.

- static const base::StringPiece new_tab_theme_css(

- ResourceBundle::GetSharedInstance().GetRawDataResource(

- IDR_NEW_INCOGNITO_TAB_THEME_CSS));

- // Create the string from our template and the replacements.

- std::string full_css =

- ui::ReplaceTemplateExpressions(new_tab_theme_css, substitutions);

- new_tab_guest_css_ = base::RefCountedString::TakeString(&full_css);

void NTPResourceCache::CreateNewTabCSS() {

ui::ThemeProvider* tp = ThemeServiceFactory::GetForProfile(profile_);

DCHECK(tp);

« no previous file with comments | « chrome/browser/ui/webui/ntp/ntp_resource_cache.h ('k') | chrome/browser/ui/webui/theme_source.cc » ('j') | chrome/browser/ui/webui/theme_source.cc » ('J')