Updates the UI and reenables the fix introduced in commit 256939

Updates the UI and reenables the fix introduced in commit 256939

This CL reenables the change introduced in http://crrev.com/256939. The UI is updated to reflect the one approved by UX team.

BUG=314621
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266474

[kaliamoorthi, 2014-04-04 15:23:32]

The CL re-enables the feature that we disabled last week.

I have made a few lines of change to fix a build failure caused after B35.

[kaliamoorthi, 2014-04-07 07:53:38]

On 2014/04/04 15:23:32, kaliamoorthi wrote:
> The CL re-enables the feature that we disabled last week.
> 
> I have made a few lines of change to fix a build failure caused after B35.

PTAL

[Patrick Dubroy, 2014-04-07 20:30:39]

https://codereview.chromium.org/225523004/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/options_ui_browsertest.cc (right):

https://codereview.chromium.org/225523004/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/options_ui_browsertest.cc:151: // Flaky on
win_rel http://crbug.com/352546
Do you know why the test is flaky? How do you know that it won't also be flaky
on other platforms?

The number of flaky tests we have is a big hit on developer productivity, and
I'm reluctant to check in a test that we already know is flaky on one platform.
Can you do some investigation to see if you can fix the problem?

[kaliamoorthi, 2014-04-11 15:06:43]

On 2014/04/07 20:30:39, Patrick Dubroy gone til Apr 22 wrote:
>
https://codereview.chromium.org/225523004/diff/20001/chrome/browser/ui/webui/...
> File chrome/browser/ui/webui/options/options_ui_browsertest.cc (right):
> 
>
https://codereview.chromium.org/225523004/diff/20001/chrome/browser/ui/webui/...
> chrome/browser/ui/webui/options/options_ui_browsertest.cc:151: // Flaky on
> win_rel http://crbug.com/352546
> Do you know why the test is flaky? How do you know that it won't also be flaky
> on other platforms?
> 
> The number of flaky tests we have is a big hit on developer productivity, and
> I'm reluctant to check in a test that we already know is flaky on one
platform.
> Can you do some investigation to see if you can fix the problem?

I analyzed this problem, and found that the tests hang in profile deletion. The
flaky 
test VerifyManagedSignout deletes the current profile. The problem happens after
the test
routine returns in the test infra-structure. There are couple of bugs which also
track the
same flaky test problem that happens in the infrastructure when profiles are
deleted.
Below are some comments from profile_manager_browsertest.cc

// This file contains tests for the ProfileManager that require a heavyweight
// InProcessBrowserTest.  These include tests involving profile deletion.

// TODO(jeremy): crbug.com/103355 - These tests should be enabled on all
// platforms.

// Delete all profiles in a multi profile setup and make sure a new one is
// created.
// Crashes/CHECKs. See crbug.com/104851

[kaliamoorthi, 2014-04-18 21:23:01]

PTAL

[kaliamoorthi, 2014-04-24 15:06:58]

I made some changes to the UI as requested by UX team and I wanted to get an
internal review first since Patrick was away.

Now that he is back, PTAL.

BTW, Patrick, I have responded to your question on flaky tests.

[Andrew T Wilson (Slow), 2014-04-24 15:46:17]

A few minor nits, but mostly looks good.

https://codereview.chromium.org/225523004/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/225523004/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:137: std::string
domain_name = "unknown";
Either leave this blank (no default domain name) or else make this a localized
string. Don't hard-code an english string here.

https://codereview.chromium.org/225523004/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:138: if (profile &&
!profile->GetProfileName().empty())
Is "profile->GetProfileName()" really what you want here? Or should you instead
grab SigninManager::GetAuthenticatedUsername()? I'm pretty sure
SigninManager::GetAuthenticatedUsername() is what you want.

https://codereview.chromium.org/225523004/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:147:
base::ASCIIToUTF16("<b>" + domain_name + "</b>")));
I'm OK with this, but I wonder if instead the <b> should be part of the
localized string?

[Patrick Dubroy, 2014-04-24 16:20:10]

Mostly LG.

https://codereview.chromium.org/225523004/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/225523004/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:147:
base::ASCIIToUTF16("<b>" + domain_name + "</b>")));
On 2014/04/24 15:46:17, Andrew T Wilson wrote:
> I'm OK with this, but I wonder if instead the <b> should be part of the
> localized string?

I don't think it should be part of the localized string. And rather than using
<b>, you should probably use <span class+"managedDomain"> or something, and use
CSS to make the text bold.

[kaliamoorthi, 2014-04-24 21:42:14]

PTAL

https://codereview.chromium.org/225523004/diff/100001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/225523004/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:137: std::string
domain_name = "unknown";
On 2014/04/24 15:46:17, Andrew T Wilson wrote:
> Either leave this blank (no default domain name) or else make this a localized
> string. Don't hard-code an english string here.

I have removed the default.

https://codereview.chromium.org/225523004/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:138: if (profile &&
!profile->GetProfileName().empty())
On 2014/04/24 15:46:17, Andrew T Wilson wrote:
> Is "profile->GetProfileName()" really what you want here? Or should you
instead
> grab SigninManager::GetAuthenticatedUsername()? I'm pretty sure
> SigninManager::GetAuthenticatedUsername() is what you want.

Done.

https://codereview.chromium.org/225523004/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:147:
base::ASCIIToUTF16("<b>" + domain_name + "</b>")));
On 2014/04/24 15:46:17, Andrew T Wilson wrote:
> I'm OK with this, but I wonder if instead the <b> should be part of the
> localized string?

I chose Patrick's solution for this one. The reason is I couldn't make the
decoration part of the localized string. The compiler complained.

https://codereview.chromium.org/225523004/diff/100001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:147:
base::ASCIIToUTF16("<b>" + domain_name + "</b>")));
On 2014/04/24 16:20:11, Patrick Dubroy wrote:
> On 2014/04/24 15:46:17, Andrew T Wilson wrote:
> > I'm OK with this, but I wonder if instead the <b> should be part of the
> > localized string?
> 
> I don't think it should be part of the localized string. And rather than using
> <b>, you should probably use <span class+"managedDomain"> or something, and
use
> CSS to make the text bold.
> 

Done.

[Patrick Dubroy, 2014-04-24 23:23:52]

https://codereview.chromium.org/225523004/diff/140001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/225523004/diff/140001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13618: +        Disconnect your Google
account
To be consistent with the other two occurrences of this string in this file, it
should be capitalized as "Disconnect your Google Account".

https://codereview.chromium.org/225523004/diff/140001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13624: +        Clear Data and Disconnect
Use sentence case: "Clear data and disconnect"

https://codereview.chromium.org/225523004/diff/140001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.html (right):

https://codereview.chromium.org/225523004/diff/140001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.html:60:
i18n-values=".innerHTML:disconnectManagedProfileDomainInformation">
Put the <span> here, and set its content via i18n-content.

https://codereview.chromium.org/225523004/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/225523004/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:135: std::string
domain_name = "<span id=disconnect-managed-profile-domain-name>" +
Sorry, I should have been more clear -- the string that gets passed to JS (via
localized_strings) shouldn't contain the <span>, that should be in the HTML.

[kaliamoorthi, 2014-04-24 23:49:22]

I need some clarifications.

https://codereview.chromium.org/225523004/diff/140001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/225523004/diff/140001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13624: +        Clear Data and Disconnect
On 2014/04/24 23:23:52, Patrick Dubroy wrote:
> Use sentence case: "Clear data and disconnect"

The one I used is the one approved by the UX team. I am not sure, if I can
change it.

https://codereview.chromium.org/225523004/diff/140001/chrome/browser/resource...
File chrome/browser/resources/options/manage_profile_overlay.html (right):

https://codereview.chromium.org/225523004/diff/140001/chrome/browser/resource...
chrome/browser/resources/options/manage_profile_overlay.html:60:
i18n-values=".innerHTML:disconnectManagedProfileDomainInformation">
On 2014/04/24 23:23:52, Patrick Dubroy wrote:
> Put the <span> here, and set its content via i18n-content.
Is it something like

i18n-values=".innerHTML:<span
id=ID>disconnectManagedProfileDomainInformation</span>

I am not sure how I can mix content and values. Can you provide some more
information, or some pointer to html source that does it.

[Andrew T Wilson (Slow), 2014-04-25 11:50:04]

lgtm

https://codereview.chromium.org/225523004/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/225523004/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:135: std::string
domain_name = "<span id=disconnect-managed-profile-domain-name>" +
On 2014/04/24 23:23:52, Patrick Dubroy wrote:
> Sorry, I should have been more clear -- the string that gets passed to JS (via
> localized_strings) shouldn't contain the <span>, that should be in the HTML.

How can he do this, given that it's embedded in another string
(MANAGED_PROFILE_DOMAIN_INFORMATION)? Are you saying we should split up
MANAGED_PROFILE_DOMAIN_INFORMATION into two strings (PRE_DOMAIN and
POST_DOMAIN)?

[Patrick Dubroy, 2014-04-25 16:38:45]

LGTM, but please change the strings as I suggested (and probably a good idea to
confirm with whomever you are working with on the UX team -- if I'm wrong, it's
an easy follow-up fix).

https://codereview.chromium.org/225523004/diff/140001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/225523004/diff/140001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13618: +        Disconnect your Google
account
On 2014/04/24 23:23:52, Patrick Dubroy wrote:
> To be consistent with the other two occurrences of this string in this file,
it
> should be capitalized as "Disconnect your Google Account".

I just confirmed with ainslie@ that it should be "Google Account".

https://codereview.chromium.org/225523004/diff/140001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13624: +        Clear Data and Disconnect
On 2014/04/24 23:49:23, kaliamoorthi wrote:
> On 2014/04/24 23:23:52, Patrick Dubroy wrote:
> > Use sentence case: "Clear data and disconnect"
> 
> The one I used is the one approved by the UX team. I am not sure, if I can
> change it.

...and confirmed that this should be sentence case.

https://codereview.chromium.org/225523004/diff/140001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/manage_profile_handler.cc (right):

https://codereview.chromium.org/225523004/diff/140001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/manage_profile_handler.cc:135: std::string
domain_name = "<span id=disconnect-managed-profile-domain-name>" +
On 2014/04/25 11:50:05, Andrew T Wilson wrote:
> On 2014/04/24 23:23:52, Patrick Dubroy wrote:
> > Sorry, I should have been more clear -- the string that gets passed to JS
(via
> > localized_strings) shouldn't contain the <span>, that should be in the HTML.
> 
> How can he do this, given that it's embedded in another string
> (MANAGED_PROFILE_DOMAIN_INFORMATION)? Are you saying we should split up
> MANAGED_PROFILE_DOMAIN_INFORMATION into two strings (PRE_DOMAIN and
> POST_DOMAIN)? 

My mistake, I didn't look closely enough. In that case, I think it's fine to
leave it as-is.

[kaliamoorthi, 2014-04-25 20:01:57]

Fixed the review comments

https://codereview.chromium.org/225523004/diff/140001/chrome/app/generated_re...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/225523004/diff/140001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13618: +        Disconnect your Google
account
On 2014/04/25 16:38:46, Patrick Dubroy wrote:
> On 2014/04/24 23:23:52, Patrick Dubroy wrote:
> > To be consistent with the other two occurrences of this string in this file,
> it
> > should be capitalized as "Disconnect your Google Account".
> 
> I just confirmed with ainslie@ that it should be "Google Account".

Done.

https://codereview.chromium.org/225523004/diff/140001/chrome/app/generated_re...
chrome/app/generated_resources.grd:13624: +        Clear Data and Disconnect
On 2014/04/25 16:38:46, Patrick Dubroy wrote:
> On 2014/04/24 23:49:23, kaliamoorthi wrote:
> > On 2014/04/24 23:23:52, Patrick Dubroy wrote:
> > > Use sentence case: "Clear data and disconnect"
> > 
> > The one I used is the one approved by the UX team. I am not sure, if I can
> > change it.
> 
> ...and confirmed that this should be sentence case.

Done.

[kaliamoorthi, 2014-04-26 04:39:57]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-04-26 04:42:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/225523004/16...

[commit-bot: I haz the power, 2014-04-26 05:25:24]

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

[commit-bot: I haz the power, 2014-04-26 05:25:25]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[kaliamoorthi, 2014-04-26 06:35:33]

Found a bug in the last minute.

The function ManageProfileHandler::GetLocalizedValues is not called if a user
signs in via the settings page. In that situation domain name is empty. The new
patch fixes it.

PTAL

[Andrew T Wilson (Slow), 2014-04-26 10:36:52]

lgtm

[Andrew T Wilson (Slow), 2014-04-26 10:39:21]

On 2014/04/26 10:36:52, Andrew T Wilson wrote:
> lgtm

Actually, looks like you have some chromeos crashes (basically, it's crashing if
you bring up the Settings page in ChromeOS guest mode because some of the
services don't exist there). Perhaps you shouldn't be initializing those strings
on ChromeOS since these buttons don't exist there?

[kaliamoorthi, 2014-04-26 15:54:01]

On 2014/04/26 10:39:21, Andrew T Wilson wrote:
> On 2014/04/26 10:36:52, Andrew T Wilson wrote:
> > lgtm
> 
> Actually, looks like you have some chromeos crashes (basically, it's crashing
if
> you bring up the Settings page in ChromeOS guest mode because some of the
> services don't exist there). Perhaps you shouldn't be initializing those
strings
> on ChromeOS since these buttons don't exist there?

Yes, that seems to be the problem. Fixed it by disabling the string
initialization for chromeos. 
The failure in UserAddingScreenTest.AddingSeveralUsers is unrelated (happens for
others without my patch as well).
Since the fix is trivial, I'll try to land it tomorrow.

[kaliamoorthi, 2014-04-26 15:54:36]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-04-26 15:55:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/225523004/20...

[commit-bot: I haz the power, 2014-04-26 16:31:59]

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

[commit-bot: I haz the power, 2014-04-26 16:31:59]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[kaliamoorthi, 2014-04-27 03:20:27]

The CQ bit was checked by kaliamoorthi@chromium.org

[commit-bot: I haz the power, 2014-04-27 03:20:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaliamoorthi@chromium.org/225523004/22...

[commit-bot: I haz the power, 2014-04-28 07:40:07]

Change committed as 266474