TPM information dialog added to EULA screen

TPM information dialog added to EULA screen
BUG=chromium-os:4945
TEST="System security setting" ling on EULA screen should show TPM dialog box OR appear disabled (grayed out) if TPM is disabled.

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

[Nikita (slow), 2010-09-24 15:07:42]

http://codereview.chromium.org/3496008/diff/8001/9001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3496008/diff/8001/9001#newcode8543
chrome/app/generated_resources.grd:8543: Your computer contains a Trusted
Platform Module (TPM) security device, which is used to implement many critical
security features in Chrome OS.\n
Should better have explicit \n\n here.

http://codereview.chromium.org/3496008/diff/8001/9003
File chrome/browser/chromeos/login/eula_view.cc (right):

http://codereview.chromium.org/3496008/diff/8001/9003#newcode127
chrome/browser/chromeos/login/eula_view.cc:127: if
(!chromeos::CryptohomeTpmIsReady) {
Should check CrosLibrary::Get()->EnsureLoaded()

http://codereview.chromium.org/3496008/diff/8001/9003#newcode129
chrome/browser/chromeos/login/eula_view.cc:129: } else if
(!chromeos::CryptohomeTpmIsReady()) {
Please file a P2 cleanup issue to move access to these functions inside
CryptohomeLibrary

http://codereview.chromium.org/3496008/diff/8001/9003#newcode302
chrome/browser/chromeos/login/eula_view.cc:302: if
(chromeos::CryptohomeTpmIsEnabled && !chromeos::CryptohomeTpmIsEnabled()) {
CrosLibrary::Get()->EnsureLoaded()

http://codereview.chromium.org/3496008/diff/8001/9003#newcode304
chrome/browser/chromeos/login/eula_view.cc:304: // TODO(glotov): add tooltip
with description.
Issue here is that if EULA screen is created when TPM is not yet ready we don't
receive notifications once it's ready to enable link.

[glotov, 2010-09-24 18:43:21]

http://codereview.chromium.org/3496008/diff/8001/9001
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/3496008/diff/8001/9001#newcode8543
chrome/app/generated_resources.grd:8543: Your computer contains a Trusted
Platform Module (TPM) security device, which is used to implement many critical
security features in Chrome OS.\n
On 2010/09/24 15:07:42, Nikita Kostylev wrote:
> Should better have explicit \n\n here.

Done.

http://codereview.chromium.org/3496008/diff/8001/9003
File chrome/browser/chromeos/login/eula_view.cc (right):

http://codereview.chromium.org/3496008/diff/8001/9003#newcode127
chrome/browser/chromeos/login/eula_view.cc:127: if
(!chromeos::CryptohomeTpmIsReady) {
On 2010/09/24 15:07:42, Nikita Kostylev wrote:
> Should check CrosLibrary::Get()->EnsureLoaded()

Done.

http://codereview.chromium.org/3496008/diff/8001/9003#newcode129
chrome/browser/chromeos/login/eula_view.cc:129: } else if
(!chromeos::CryptohomeTpmIsReady()) {
On 2010/09/24 15:07:42, Nikita Kostylev wrote:
> Please file a P2 cleanup issue to move access to these functions inside
> CryptohomeLibrary

Done.

http://codereview.chromium.org/3496008/diff/8001/9003#newcode302
chrome/browser/chromeos/login/eula_view.cc:302: if
(chromeos::CryptohomeTpmIsEnabled && !chromeos::CryptohomeTpmIsEnabled()) {
On 2010/09/24 15:07:42, Nikita Kostylev wrote:
> CrosLibrary::Get()->EnsureLoaded()

Done.

http://codereview.chromium.org/3496008/diff/8001/9003#newcode304
chrome/browser/chromeos/login/eula_view.cc:304: // TODO(glotov): add tooltip
with description.
I thought that if TPM were not enabled, it will not become enabled during this
session, no?

On 2010/09/24 15:07:42, Nikita Kostylev wrote:
> Issue here is that if EULA screen is created when TPM is not yet ready we
don't
> receive notifications once it's ready to enable link.

[Nikita (slow), 2010-09-27 10:19:06]

http://codereview.chromium.org/3496008/diff/8001/9003
File chrome/browser/chromeos/login/eula_view.cc (right):

http://codereview.chromium.org/3496008/diff/8001/9003#newcode129
chrome/browser/chromeos/login/eula_view.cc:129: } else if
(!chromeos::CryptohomeTpmIsReady()) {
On 2010/09/24 18:43:21, glotov wrote:
> On 2010/09/24 15:07:42, Nikita Kostylev wrote:
> > Please file a P2 cleanup issue to move access to these functions inside
> > CryptohomeLibrary
> 
> Done.

Thanks!

http://codereview.chromium.org/3496008/diff/8001/9003#newcode304
chrome/browser/chromeos/login/eula_view.cc:304: // TODO(glotov): add tooltip
with description.
On 2010/09/24 18:43:21, glotov wrote:
> I thought that if TPM were not enabled, it will not become enabled during this
> session, no?
> 
> On 2010/09/24 15:07:42, Nikita Kostylev wrote:
> > Issue here is that if EULA screen is created when TPM is not yet ready we
> don't
> > receive notifications once it's ready to enable link.
> 
> 

Sounds reasonable, assuming that TPM might be disabled only when it's absent or
when it's been explicitly disabled via BIOS.

http://codereview.chromium.org/3496008/diff/14001/15003#newcode129
chrome/browser/chromeos/login/eula_view.cc:129: } else if
(!chromeos::CryptohomeTpmIsReady()) {
It should not be DCHECK though. We test for
chromeos::CrosLibrary::Get()->EnsureLoaded() in all places where we use
CrosLibrary. If it fails for some reason code needs to failback.

http://codereview.chromium.org/3496008/diff/14001/15003#newcode134
chrome/browser/chromeos/login/eula_view.cc:134: }
On the second though I don't think we should check whether TPM is ready here.

Let's check it when Link is clicked and in case when it's not ready show error
bubble with the short message explaining that. 

I suppose that TPM being slow is a normal situation and it's not a LOG(ERROR)
only.

[glotov, 2010-09-27 17:18:17]

All done, please have a look again

[Nikita (slow), 2010-09-27 17:51:01]

LGTM

http://codereview.chromium.org/3496008/diff/21001/22003
File chrome/browser/chromeos/login/eula_view.cc (right):

http://codereview.chromium.org/3496008/diff/21001/22003#newcode394
chrome/browser/chromeos/login/eula_view.cc:394: if (!bubble_)
indent

http://codereview.chromium.org/3496008/diff/21001/22004
File chrome/browser/chromeos/login/eula_view.h (right):

http://codereview.chromium.org/3496008/diff/21001/22004#newcode126
chrome/browser/chromeos/login/eula_view.h:126: virtual void
OnHelpLinkActivated() { }
{}

[glotov, 2010-09-27 18:52:00]

http://codereview.chromium.org/3496008/diff/21001/22003
File chrome/browser/chromeos/login/eula_view.cc (right):

http://codereview.chromium.org/3496008/diff/21001/22003#newcode394
chrome/browser/chromeos/login/eula_view.cc:394: if (!bubble_)
On 2010/09/27 17:51:01, Nikita Kostylev wrote:
> indent

Done.

http://codereview.chromium.org/3496008/diff/21001/22004
File chrome/browser/chromeos/login/eula_view.h (right):

http://codereview.chromium.org/3496008/diff/21001/22004#newcode126
chrome/browser/chromeos/login/eula_view.h:126: virtual void
OnHelpLinkActivated() { }
On 2010/09/27 17:51:01, Nikita Kostylev wrote:
> {}

Done.