Issue 1440113002: [MD] Change appearance of incognito NTP to MD spec.

Evan Stade

Description was changed from ========== [MD] Change appearance of incognito NTP to MD spec. - ...

5 years, 1 month ago (2015-11-13 01:18:39 UTC) #1

Evan Stade

estade@chromium.org changed reviewers: + dbeam@chromium.org

5 years, 1 month ago (2015-11-13 01:18:39 UTC) #2

Dan Beam

Description was changed from ========== [MD] Change appearance of incognito NTP to MD spec. OTR: ...

5 years, 1 month ago (2015-11-13 01:25:00 UTC) #4

Dan Beam

can I have screenshots? also, I see your TODO: will this be addressed by the ...

5 years, 1 month ago (2015-11-13 01:32:22 UTC) #5

Dan Beam

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui/theme_source.cc File chrome/browser/ui/webui/theme_source.cc (right): https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui/theme_source.cc#newcode71 chrome/browser/ui/webui/theme_source.cc:71: // NB: it's important that this is |profile| and ...

5 years, 1 month ago (2015-11-13 01:33:23 UTC) #6

Evan Stade

On 2015/11/13 01:32:22, Dan Beam wrote: > can I have screenshots? also, I see your ...

5 years, 1 month ago (2015-11-13 01:39:00 UTC) #7

On 2015/11/13 01:32:22, Dan Beam wrote:
> can I have screenshots?  also, I see your TODO: will this be addressed by the
> branchpoint?

I can definitely wait to land this after the branch point. No rush in that
regard.

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:272: if (!new_tab_css_.get())
On 2015/11/13 01:32:22, Dan Beam wrote:
> nit: .get() isn't necessary here

Acknowledged (will fix).

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:558: ?
".doesNotHaveCustomBackground"
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.

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.

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/theme_source.cc (right):

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/theme_source.cc:71: // NB: it's important that this is
|profile| and not |profile_|.
On 2015/11/13 01:33:23, Dan Beam wrote:
> On 2015/11/13 01:32:22, Dan Beam wrote:
> > random question: is it too weird to just initialize profile_ afterward?  as
> in:
> > move it out of the initializer list?
> 
> well
> 
>   : profile_(nullptr)
> 
>   ...
> 
>   profile_ = profile->GetOriginalProfile();

I guess that would be one way to make this more bullet proof. Although then
someone would come along and ask "why isn't this just in the initializer list?"

Dan Beam

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc#newcode558 chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:558: ? ".doesNotHaveCustomBackground" On 2015/11/13 01:39:00, Evan Stade wrote: > ...

5 years, 1 month ago (2015-11-16 19:57:25 UTC) #9

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:558: ?
".doesNotHaveCustomBackground"
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?
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

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/theme_source.cc (right):

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/theme_source.cc:71: // NB: it's important that this is
|profile| and not |profile_|.
On 2015/11/13 01:39:00, Evan Stade wrote:
> On 2015/11/13 01:33:23, Dan Beam wrote:
> > On 2015/11/13 01:32:22, Dan Beam wrote:
> > > random question: is it too weird to just initialize profile_ afterward? 
as
> > in:
> > > move it out of the initializer list?
> > 
> > well
> > 
> >   : profile_(nullptr)
> > 
> >   ...
> > 
> >   profile_ = profile->GetOriginalProfile();
> 
> I guess that would be one way to make this more bullet proof. Although then
> someone would come along and ask "why isn't this just in the initializer
list?"

ok

Evan Stade

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc#newcode558 chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:558: ? ".doesNotHaveCustomBackground" On 2015/11/16 19:57:25, Dan Beam wrote: > ...

5 years, 1 month ago (2015-11-16 20:10:08 UTC) #10

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:558: ?
".doesNotHaveCustomBackground"
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.

>
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

Dan Beam

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc#newcode558 chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:558: ? ".doesNotHaveCustomBackground" On 2015/11/16 20:10:08, Evan Stade wrote: > ...

5 years, 1 month ago (2015-11-17 00:18:59 UTC) #11

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:558: ?
".doesNotHaveCustomBackground"
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
>

Dan Beam

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc#newcode558 chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:558: ? ".doesNotHaveCustomBackground" On 2015/11/17 00:18:59, Dan Beam wrote: > ...

5 years, 1 month ago (2015-11-17 00:26:15 UTC) #12

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:558: ?
".doesNotHaveCustomBackground"
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

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right): https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui/ntp/ntp_resource_cache.cc#newcode272 chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:272: if (!new_tab_css_.get()) On 2015/11/13 01:39:00, Evan Stade wrote: > ...

5 years, 1 month ago (2015-11-19 23:05:24 UTC) #13

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:272: if (!new_tab_css_.get())
On 2015/11/13 01:39:00, Evan Stade wrote:
> On 2015/11/13 01:32:22, Dan Beam wrote:
> > nit: .get() isn't necessary here
> 
> Acknowledged (will fix).

Done.

https://codereview.chromium.org/1440113002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:558: ?
".doesNotHaveCustomBackground"
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.

Evan Stade

The CQ bit was checked by estade@chromium.org to run a CQ dry run

5 years, 1 month ago (2015-11-19 23:05:32 UTC) #14

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440113002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440113002/60001

5 years, 1 month ago (2015-11-19 23:08:34 UTC) #15

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 1 month ago (2015-11-19 23:19:14 UTC) #16

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/125687)

5 years, 1 month ago (2015-11-19 23:19:15 UTC) #17

Dan Beam

looks pretty good to me https://codereview.chromium.org/1440113002/diff/60001/chrome/browser/resources/ntp4/incognito_and_guest_tab.css File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): https://codereview.chromium.org/1440113002/diff/60001/chrome/browser/resources/ntp4/incognito_and_guest_tab.css#newcode39 chrome/browser/resources/ntp4/incognito_and_guest_tab.css:39: html[hasCustomBackground='true'] .content { nit: ...

5 years, 1 month ago (2015-11-20 00:48:45 UTC) #18

Dan Beam

https://codereview.chromium.org/1440113002/diff/60001/chrome/browser/resources/ntp4/incognito_tab.html File chrome/browser/resources/ntp4/incognito_tab.html (right): https://codereview.chromium.org/1440113002/diff/60001/chrome/browser/resources/ntp4/incognito_tab.html#newcode45 chrome/browser/resources/ntp4/incognito_tab.html:45: /** @param {!{hasCustomBackground: boolean}} themeData */

5 years, 1 month ago (2015-11-20 00:50:31 UTC) #19

Evan Stade

The CQ bit was checked by estade@chromium.org to run a CQ dry run

5 years, 1 month ago (2015-11-20 22:02:56 UTC) #20

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440113002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440113002/80001

5 years, 1 month ago (2015-11-20 22:04:59 UTC) #21

Evan Stade

The CQ bit was checked by estade@chromium.org to run a CQ dry run

5 years, 1 month ago (2015-11-20 22:05:14 UTC) #22

Evan Stade

https://codereview.chromium.org/1440113002/diff/60001/chrome/browser/resources/ntp4/incognito_and_guest_tab.css File chrome/browser/resources/ntp4/incognito_and_guest_tab.css (right): https://codereview.chromium.org/1440113002/diff/60001/chrome/browser/resources/ntp4/incognito_and_guest_tab.css#newcode39 chrome/browser/resources/ntp4/incognito_and_guest_tab.css:39: html[hasCustomBackground='true'] .content { On 2015/11/20 00:48:45, Dan Beam wrote: ...

5 years, 1 month ago (2015-11-20 22:05:17 UTC) #23

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440113002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440113002/100001

5 years, 1 month ago (2015-11-20 22:07:07 UTC) #24

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 1 month ago (2015-11-20 22:17:32 UTC) #25

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/126245) mac_chromium_rel_ng on ...

5 years, 1 month ago (2015-11-20 22:17:33 UTC) #26

Dan Beam

lgtm https://codereview.chromium.org/1440113002/diff/100001/chrome/browser/resources/ntp4/incognito_tab.css File chrome/browser/resources/ntp4/incognito_tab.css (right): https://codereview.chromium.org/1440113002/diff/100001/chrome/browser/resources/ntp4/incognito_tab.css#newcode11 chrome/browser/resources/ntp4/incognito_tab.css:11: content: url(../../../../ui/webui/resources/images/incognito_splash.svg); git add ui/webui/resources/images/incognito_splash.svg

5 years, 1 month ago (2015-11-21 00:31:42 UTC) #27

Evan Stade

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the ...

5 years, 1 month ago (2015-11-21 00:36:32 UTC) #29

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440113002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440113002/120001

5 years, 1 month ago (2015-11-21 00:37:04 UTC) #30

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 1 month ago (2015-11-21 01:18:56 UTC) #31

commit-bot: I haz the power

Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/144165)

5 years, 1 month ago (2015-11-21 01:18:56 UTC) #32

Evan Stade

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the ...

5 years, 1 month ago (2015-11-23 19:06:39 UTC) #34

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440113002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440113002/140001

5 years, 1 month ago (2015-11-23 19:09:12 UTC) #35

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 1 month ago (2015-11-23 19:47:27 UTC) #36

commit-bot: I haz the power

Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/92188) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, ...

5 years, 1 month ago (2015-11-23 19:47:28 UTC) #37

Evan Stade

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the ...

5 years, 1 month ago (2015-11-24 03:17:41 UTC) #39

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440113002/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440113002/170001

5 years, 1 month ago (2015-11-24 03:20:10 UTC) #40

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

5 years, 1 month ago (2015-11-24 03:57:30 UTC) #41

commit-bot: I haz the power

Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/145090) win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, ...

5 years, 1 month ago (2015-11-24 03:57:31 UTC) #42

Evan Stade

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org Link to the ...

5 years ago (2015-11-24 21:25:25 UTC) #44

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1440113002/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1440113002/190001

5 years ago (2015-11-24 21:27:04 UTC) #45

commit-bot: I haz the power

5 years ago (2015-11-25 00:45:35 UTC) #47

Message was sent while issue was closed.

Patchset 11 (id:??) landed as
https://crrev.com/b561e0dc52b257b98e0f8a1373dd7a744b69b63d
Cr-Commit-Position: refs/heads/master@{#361499}

Issue 1440113002: [MD] Change appearance of incognito NTP to MD spec. (Closed)

Description

Patch Set 1 #

Patch Set 2 : guest #

Patch Set 3 : rebase and change stuff #

Patch Set 4 : cant remember if i changed anything #

Patch Set 5 : dbeam review #

Patch Set 6 : . #

Patch Set 7 : git add my little friend #

Patch Set 8 : fix typo #

Patch Set 9 : no themes in guest #

Patch Set 10 : reimprove small width window layout #

Patch Set 11 : . #

Messages