Show GAIA profile info in NTP

Show GAIA profile info in NTP

Screenshots: http://imgur.com/6owxK

BUG=91241
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112876

[Robert Sesek, 2011-11-28 23:05:44]

C++ lgtm, but estade should have final say

[Evan Stade, 2011-11-29 02:27:48]

http://codereview.chromium.org/8680033/diff/6001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.css (right):

http://codereview.chromium.org/8680033/diff/6001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.css:210: /* Draw a border inside the icon
by using an inset shadow. */
why can't you use an actual border?

http://codereview.chromium.org/8680033/diff/6001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.css:211: -webkit-box-shadow: inset 0px 0px
1px black;
in this file, -webkit-foo comes at the end of the css block (best to remain
locally consistent, although if you want to change the entire file to have
-webkit-foo at the beginning that works too)

http://codereview.chromium.org/8680033/diff/6001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.html (right):

http://codereview.chromium.org/8680033/diff/6001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.html:79: <img id="login-status-icon">
use css background

[sail, 2011-11-29 19:11:03]

http://codereview.chromium.org/8680033/diff/6001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.css (right):

http://codereview.chromium.org/8680033/diff/6001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.css:210: /* Draw a border inside the icon
by using an inset shadow. */
On 2011/11/29 02:27:48, Evan Stade wrote:
> why can't you use an actual border?

I need to draw the border inside the image. I couldn't figure out a way to do
this with a border.

http://codereview.chromium.org/8680033/diff/6001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.css:211: -webkit-box-shadow: inset 0px 0px
1px black;
On 2011/11/29 02:27:48, Evan Stade wrote:
> in this file, -webkit-foo comes at the end of the css block (best to remain
> locally consistent, although if you want to change the entire file to have
> -webkit-foo at the beginning that works too)

Done.

http://codereview.chromium.org/8680033/diff/6001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.html (right):

http://codereview.chromium.org/8680033/diff/6001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.html:79: <img id="login-status-icon">
On 2011/11/29 02:27:48, Evan Stade wrote:
> use css background

working on it

[sail, 2011-11-30 17:48:33]

http://codereview.chromium.org/8680033/diff/6001/chrome/browser/resources/ntp...
File chrome/browser/resources/ntp4/new_tab.html (right):

http://codereview.chromium.org/8680033/diff/6001/chrome/browser/resources/ntp...
chrome/browser/resources/ntp4/new_tab.html:79: <img id="login-status-icon">
On 2011/11/29 02:27:48, Evan Stade wrote:
> use css background

Are you ok with me using a shadow to do an inset border.
With the shadow method you need two elements, the element containing the image
has to be lower in the zorder.

If you're ok with this method then it seems like leaving this as an image
element would be better than using a 2nd div with a css background.

[sail, 2011-12-01 02:43:57]

I removed the shadow style. Please take another look. Thanks.

[Evan Stade, 2011-12-02 00:03:26]

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.css (right):

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:198: #login-status-header-container {
I meant to suggest this should be consolidated with the other rule block, just
use a comma in between selectors

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:209: #login-status-icon {
so no border then? if no border, then I guess we don't need this div.

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:211: width: 27px;
alphabet

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:362: function updateLogin(loginHeader,
loginSubHeader, icon_url) {
camel case and documentation

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:370: console.log("setting icon");
debug output?

[sail, 2011-12-02 01:19:57]

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.css (right):

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:198: #login-status-header-container {
On 2011/12/02 00:03:26, Evan Stade wrote:
> I meant to suggest this should be consolidated with the other rule block, just
> use a comma in between selectors

Sorry, consolidated with which rule block?

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:209: #login-status-icon {
On 2011/12/02 00:03:26, Evan Stade wrote:
> so no border then? if no border, then I guess we don't need this div.

I don't think I understand. Don't I still need an element to display the icon
in?

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:211: width: 27px;
On 2011/12/02 00:03:26, Evan Stade wrote:
> alphabet

Done.

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:362: function updateLogin(loginHeader,
loginSubHeader, icon_url) {
On 2011/12/02 00:03:26, Evan Stade wrote:
> camel case and documentation

Done.

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:370: console.log("setting icon");
On 2011/12/02 00:03:26, Evan Stade wrote:
> debug output?

Done.

[Evan Stade, 2011-12-02 02:25:16]

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.css (right):

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:198: #login-status-header-container {
On 2011/12/02 01:19:57, sail wrote:
> On 2011/12/02 00:03:26, Evan Stade wrote:
> > I meant to suggest this should be consolidated with the other rule block,
just
> > use a comma in between selectors
> 
> Sorry, consolidated with which rule block?

#login-status-action-area (in fact it's probably a good idea to create a class
instead of calling them both out by ID)

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:209: #login-status-icon {
On 2011/12/02 01:19:57, sail wrote:
> On 2011/12/02 00:03:26, Evan Stade wrote:
> > so no border then? if no border, then I guess we don't need this div.
> 
> I don't think I understand. Don't I still need an element to display the icon
> in?

make it a background image of login-status-header-container or
login-status-header

[sail, 2011-12-02 18:42:28]

New screenshots:
  left to right: http://imgur.com/7Tk1Z,qndhZ#0
  right to left: http://imgur.com/7Tk1Z,qndhZ#1

Much simpler code. Please take a look!

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.css (right):

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:198: #login-status-header-container {
On 2011/12/02 02:25:16, Evan Stade wrote:
> On 2011/12/02 01:19:57, sail wrote:
> > On 2011/12/02 00:03:26, Evan Stade wrote:
> > > I meant to suggest this should be consolidated with the other rule block,
> just
> > > use a comma in between selectors
> > 
> > Sorry, consolidated with which rule block?
> 
> #login-status-action-area (in fact it's probably a good idea to create a class
> instead of calling them both out by ID)

Done.

http://codereview.chromium.org/8680033/diff/10001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:209: #login-status-icon {
On 2011/12/02 02:25:16, Evan Stade wrote:
> On 2011/12/02 01:19:57, sail wrote:
> > On 2011/12/02 00:03:26, Evan Stade wrote:
> > > so no border then? if no border, then I guess we don't need this div.
> > 
> > I don't think I understand. Don't I still need an element to display the
icon
> > in?
> 
> make it a background image of login-status-header-container or
> login-status-header

Done.

[Evan Stade, 2011-12-02 21:02:16]

just nits, lgtm otherwise

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.css (right):

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:199: background-repeat:no-repeat;
space

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:202: padding-right: 37px;
-webkit-padding-end

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:206: background-position: left center;
nit: only change background-position-x

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:227: .login-status-vcenter {
can you come up with a better name? login-status-row or something.

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:375:
$('login-status-header-container').style.backgroundImage = "none";
single quotes

[sail, 2011-12-03 00:09:00]

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.css (right):

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:199: background-repeat:no-repeat;
On 2011/12/02 21:02:16, Evan Stade wrote:
> space

Done.

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:202: padding-right: 37px;
On 2011/12/02 21:02:16, Evan Stade wrote:
> -webkit-padding-end

Done.

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:206: background-position: left center;
On 2011/12/02 21:02:16, Evan Stade wrote:
> nit: only change background-position-x

Done.

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.css:227: .login-status-vcenter {
On 2011/12/02 21:02:16, Evan Stade wrote:
> can you come up with a better name? login-status-row or something.

Done.

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
File chrome/browser/resources/ntp4/new_tab.js (right):

http://codereview.chromium.org/8680033/diff/17001/chrome/browser/resources/nt...
chrome/browser/resources/ntp4/new_tab.js:375:
$('login-status-header-container').style.backgroundImage = "none";
On 2011/12/02 21:02:16, Evan Stade wrote:
> single quotes

Done.

[commit-bot: I haz the power, 2011-12-03 00:09:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8680033/22001

[commit-bot: I haz the power, 2011-12-03 02:37:35]

Try job failure for 8680033-22001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-03 05:04:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/8680033/26001

[commit-bot: I haz the power, 2011-12-03 06:55:39]

Change committed as 112876