Profile/user menu on NTP should look more clickable?

Profile/user menu on NTP should look more clickable?

BUG=102685
TEST=NONE
R= estade@chromium.org, sail@chromium.org, jeffreyc@chromium.org

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

[Tyler Breisacher (Chromium), 2011-12-06 18:51:23]

Just adding a :hover CSS rule so the user's name is more clickable-looking...

[sail, 2011-12-06 19:02:41]

Also, make sure to include screenshots when making visual changes. You'll
probably want to get feedback from Jeff on the final look of the UI.

http://codereview.chromium.org/8822024/diff/1/chrome/browser/resources/ntp4/n...
File chrome/browser/resources/ntp4/new_tab.css (right):

http://codereview.chromium.org/8822024/diff/1/chrome/browser/resources/ntp4/n...
chrome/browser/resources/ntp4/new_tab.css:194: text-decoration: underline;
Currently we use the login-container to show two different things:
  #1 To show the email address of the logged in user
  #2 If no user is logged in then we use it to prompt the user to login:
        Not signed in to Chrome
        (You're missing out -- sign in)

I think we only want to put the underline in case #1.

Doing this should be fairly simple.  Just add a new class to the email address
in this code:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/web...

See this for a good example on how to do that:
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/web...

[Tyler Breisacher (Chromium), 2011-12-06 19:33:22]

http://codereview.chromium.org/8822024/diff/1/chrome/browser/resources/ntp4/n...
File chrome/browser/resources/ntp4/new_tab.css (right):

http://codereview.chromium.org/8822024/diff/1/chrome/browser/resources/ntp4/n...
chrome/browser/resources/ntp4/new_tab.css:194: text-decoration: underline;
for #1, I'm seeing my email address on my installed Chrome (16.0.912.59 beta)
but my name on Chromium built from source (17.0.964.0 (Developer Build
113232-dirty Linux)).
for #2, I'm seeing that on Chrome but not seeing anything on Chromium. Is this
actually a Chrome/Chromium difference, or do I need to change a preference to
make it show up the way you're describing?

[sail, 2011-12-06 19:41:43]

http://codereview.chromium.org/8822024/diff/1/chrome/browser/resources/ntp4/n...
File chrome/browser/resources/ntp4/new_tab.css (right):

http://codereview.chromium.org/8822024/diff/1/chrome/browser/resources/ntp4/n...
chrome/browser/resources/ntp4/new_tab.css:194: text-decoration: underline;
On 2011/12/06 19:33:22, tbreisacher wrote:
> for #1, I'm seeing my email address on my installed Chrome (16.0.912.59 beta)
> but my name on Chromium built from source (17.0.964.0 (Developer Build
> 113232-dirty Linux)).

Yea, this is a new change on trunk that uses your full name from your Google
Account. Making that underlined on hover still seems like a good idea (though
definitely run it by Jeff).

> for #2, I'm seeing that on Chrome but not seeing anything on Chromium. Is this
> actually a Chrome/Chromium difference, or do I need to change a preference to
> make it show up the way you're describing?

We enable/disable this using a server side check. To force it to show just
change this else if statement:
     
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/ui/web...
to just be true.

[Evan Stade, 2011-12-06 20:21:39]

http://codereview.chromium.org/8822024/diff/1/chrome/browser/resources/ntp4/n...
File chrome/browser/resources/ntp4/new_tab.css (right):

http://codereview.chromium.org/8822024/diff/1/chrome/browser/resources/ntp4/n...
chrome/browser/resources/ntp4/new_tab.css:187: text-decoration: none;
i dont think you need this any more?

[Tyler Breisacher (Chromium), 2011-12-06 22:49:59]

I uploaded a new CL which I hope is at least a step closer to what we want. :)
I'll add some screenshots too. As far as I know, Rietveld doesn't support that
so I should upload them to crbug.com/102685 instead?

http://codereview.chromium.org/8822024/diff/1/chrome/browser/resources/ntp4/n...
File chrome/browser/resources/ntp4/new_tab.css (right):

http://codereview.chromium.org/8822024/diff/1/chrome/browser/resources/ntp4/n...
chrome/browser/resources/ntp4/new_tab.css:187: text-decoration: none;
Good point. In fact, as far as I can tell we never needed it.

[sail, 2011-12-06 23:00:57]

LGTM!

make sure to get an lgtm from jeff and estade as well.

you can post screenshots anywhere. I use imgur but any site is fine.

http://codereview.chromium.org/8822024/diff/6001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

http://codereview.chromium.org/8822024/diff/6001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:190: net::EscapeForHTML(name) +
Ahh, I didn't know about EscapeForHTML(). Can you do the same thing for
signed_in_link (below)

http://codereview.chromium.org/8822024/diff/6001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:198: header =
UTF8ToUTF16(username);
You'll need to modify this case as well

[Tyler Breisacher (Chromium), 2011-12-06 23:17:47]

http://codereview.chromium.org/8822024/diff/6001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

http://codereview.chromium.org/8822024/diff/6001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:190: net::EscapeForHTML(name) +
I figured it's unlikely someone would say their name was, for example, "Sailesh
<script src="example.com/evil.js"><script> Agrawal" but better safe than sorry
:) Will do.

[Tyler Breisacher (Chromium), 2011-12-06 23:24:50]

Adding jeffreyc to the review...

[sail, 2011-12-06 23:46:25]

http://codereview.chromium.org/8822024/diff/6001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

http://codereview.chromium.org/8822024/diff/6001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:198: header =
UTF8ToUTF16(username);
On 2011/12/06 23:00:57, sail wrote:
> You'll need to modify this case as well

Sorry, I mean you'll need to add "class='profile-name'" to this as well.

[Evan Stade, 2011-12-08 18:23:34]

lgtm

http://codereview.chromium.org/8822024/diff/6006/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

http://codereview.chromium.org/8822024/diff/6006/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:197: if (header.empty())
curlies

[jeffreyc, 2011-12-08 20:28:50]

LGTM

On Thu, Dec 8, 2011 at 10:23 AM, <estade@chromium.org> wrote:

> lgtm
>
>
> http://codereview.chromium.**org/8822024/diff/6006/chrome/**
>
browser/ui/webui/ntp/ntp_**login_handler.cc<http://codereview.chromium.org/8822024/diff/6006/chrome/browser/ui/webui/ntp/ntp_login_handler.cc>
> File chrome/browser/ui/webui/ntp/**ntp_login_handler.cc (right):
>
> http://codereview.chromium.**org/8822024/diff/6006/chrome/**
>
browser/ui/webui/ntp/ntp_**login_handler.cc#newcode197<http://codereview.chromium.org/8822024/diff/6006/chrome/browser/ui/webui/ntp/ntp_login_handler.cc#newcode197>
> chrome/browser/ui/webui/ntp/**ntp_login_handler.cc:197: if
> (header.empty())
> curlies
>
>
http://codereview.chromium.**org/8822024/<http://codereview.chromium.org/8822...
>

[commit-bot: I haz the power, 2011-12-08 20:47:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbreisacher@chromium.org/8822024/12001

[commit-bot: I haz the power, 2011-12-08 22:51:38]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_rel, revision is 113653, job name
was 8822024-12001 (previous was lost) (previous was lost) (previous was lost)
(retry).

[commit-bot: I haz the power, 2011-12-09 18:18:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbreisacher@chromium.org/8822024/12001

[commit-bot: I haz the power, 2011-12-09 22:19:33]

Change committed as 113862

[James Hawkins, 2011-12-10 03:07:30]

http://codereview.chromium.org/8822024/diff/12001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

http://codereview.chromium.org/8822024/diff/12001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:190: net::EscapeForHTML(name) +
nit: Wrapped lines must be indented exactly 4 spaces.

http://codereview.chromium.org/8822024/diff/12001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:198: header =
UTF8ToUTF16("<span class='profile-name'>" +
You use ASCIITo above and UTF8To here; is there a reason for the difference?

[Tyler Breisacher (Chromium), 2011-12-10 03:43:25]

http://codereview.chromium.org/8822024/diff/12001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

http://codereview.chromium.org/8822024/diff/12001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:198: header =
UTF8ToUTF16("<span class='profile-name'>" +
I was doing ASCII above because I was following the example at line 207
(signed_in_link = ...), but then I did UTF8 here, because the UTF8 function was
already being used, presumably because we know the username to be UTF8, but we
don't know it to be ASCII.

But I agree it's confusing. Do you think we should change all three spots in
this function to use UTF8, to be consistent?

[James Hawkins, 2011-12-10 04:14:24]

http://codereview.chromium.org/8822024/diff/12001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_login_handler.cc (right):

http://codereview.chromium.org/8822024/diff/12001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_login_handler.cc:198: header =
UTF8ToUTF16("<span class='profile-name'>" +
On 2011/12/10 03:43:25, tbreisacher wrote:
> I was doing ASCII above because I was following the example at line 207
> (signed_in_link = ...), but then I did UTF8 here, because the UTF8 function
was
> already being used, presumably because we know the username to be UTF8, but we
> don't know it to be ASCII.
> 
> But I agree it's confusing. Do you think we should change all three spots in
> this function to use UTF8, to be consistent?

Probably.  If these are all the same, we should refactor this into a method.