chromeos: Remove CryptohomeLibrary::TpmGetPassword and TpmIsReady

chromeos: Remove CryptohomeLibrary::TpmGetPassword and TpmIsReady

Use CrypthomeClient instead.
Also make the methods asynchronous at the same time.

BUG=126674, 126719
TEST=Open chrome://cryptohome

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

[stevenjb, 2012-07-12 15:32:01]

lgtm, but I'd really like to see us follow up with crbug.com/126674 cleanup
soon.

http://codereview.chromium.org/10703162/diff/7002/chrome/browser/ui/webui/abo...
File chrome/browser/ui/webui/about_ui.cc (right):

http://codereview.chromium.org/10703162/diff/7002/chrome/browser/ui/webui/abo...
chrome/browser/ui/webui/about_ui.cc:524: class CryptohomeDataRequest : public
base::RefCounted<CryptohomeDataRequest> {
This seems like it belongs in its own file, not about_ui.cc, even if it is
currently only used here.

http://codereview.chromium.org/10703162/diff/7002/chrome/browser/ui/webui/abo...
chrome/browser/ui/webui/about_ui.cc:549: // TODO(hashimoto): Get these values
asynchronously. crbug.com/126674
We could, however, wait to move this until addressing crbug.com/126674.

http://codereview.chromium.org/10703162/diff/7002/chrome/browser/ui/webui/abo...
chrome/browser/ui/webui/about_ui.cc:561: const
chromeos::CryptohomeClient::BoolMethodCallback&);
This is extremely difficult to read/parse. I don't suppose we could simplify it?
Or create a helper class instead? Again, that could wait until the rest of the
class is made asynchronous and moved.

http://codereview.chromium.org/10703162/diff/7002/chrome/browser/ui/webui/abo...
chrome/browser/ui/webui/about_ui.cc:572: destination));
Also very confusing.

[hashimoto, 2012-07-13 04:27:21]

Evan, could you review ui/webui/about_ui.cc as an owner?

https://chromiumcodereview.appspot.com/10703162/diff/7002/chrome/browser/ui/w...
File chrome/browser/ui/webui/about_ui.cc (right):

https://chromiumcodereview.appspot.com/10703162/diff/7002/chrome/browser/ui/w...
chrome/browser/ui/webui/about_ui.cc:549: // TODO(hashimoto): Get these values
asynchronously. crbug.com/126674
On 2012/07/12 15:32:02, stevenjb (chromium) wrote:
> We could, however, wait to move this until addressing crbug.com/126674.

Yes, let's fix crbug.com/126674 first.

https://chromiumcodereview.appspot.com/10703162/diff/7002/chrome/browser/ui/w...
chrome/browser/ui/webui/about_ui.cc:561: const
chromeos::CryptohomeClient::BoolMethodCallback&);
On 2012/07/12 15:32:02, stevenjb (chromium) wrote:
> This is extremely difficult to read/parse. I don't suppose we could simplify
it?
> Or create a helper class instead? Again, that could wait until the rest of the
> class is made asynchronous and moved.

Agree, member-function-pointer and operator '->*' are ones of the trickiest
parts of C++.

[Evan Stade, 2012-07-13 05:58:46]

http://codereview.chromium.org/10703162/diff/10002/chrome/browser/ui/webui/ab...
File chrome/browser/ui/webui/about_ui.cc (right):

http://codereview.chromium.org/10703162/diff/10002/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:1403: }  // namespace
wow. 1400 line unnamed namespace? html in C++ in string literals? this file
really sucks.  I won't ask you to fix the file as whole, but you could bring
cryptohome out of the ice age by making it follow the general design principles
of webui (separated html/js/css, data passed via json, etc.)

[hashimoto, 2012-07-18 09:34:21]

New patch set, added cryptohome_ui.cc.
PTAL.

http://codereview.chromium.org/10703162/diff/10002/chrome/browser/ui/webui/ab...
File chrome/browser/ui/webui/about_ui.cc (right):

http://codereview.chromium.org/10703162/diff/10002/chrome/browser/ui/webui/ab...
chrome/browser/ui/webui/about_ui.cc:1403: }  // namespace
On 2012/07/13 05:58:46, Evan Stade wrote:
> wow. 1400 line unnamed namespace? html in C++ in string literals? this file
> really sucks.  I won't ask you to fix the file as whole, but you could bring
> cryptohome out of the ice age by making it follow the general design
principles
> of webui (separated html/js/css, data passed via json, etc.)

Now chrome://cryptohome (and about:cryptohome) is handled with newly added
CryptohomeUI.

[Evan Stade, 2012-07-19 01:53:16]

thanks!

http://codereview.chromium.org/10703162/diff/15002/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/cryptohome_ui.cc (right):

http://codereview.chromium.org/10703162/diff/15002/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/cryptohome_ui.cc:36: CryptohomeWebUIHandler() :
weak_ptr_factory_(this) {}
this gets its own file please

http://codereview.chromium.org/10703162/diff/15002/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/cryptohome_ui.cc:61:
SetCryptohomeBoolProperty(destination_id,
all this should just be stuffed into loadTimeData. on the C++ side it looks like

  source->AddString(...)

in CreateCryptohomeUIHTMLSource. For booleans you can either add to the
localized_strings Dictionary directly or you could add a AddBoolean function
that parallels AddString.

[hashimoto, 2012-07-19 07:14:54]

https://chromiumcodereview.appspot.com/10703162/diff/15002/chrome/browser/ui/...
File chrome/browser/ui/webui/chromeos/cryptohome_ui.cc (right):

https://chromiumcodereview.appspot.com/10703162/diff/15002/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/cryptohome_ui.cc:36: CryptohomeWebUIHandler() :
weak_ptr_factory_(this) {}
On 2012/07/19 01:53:16, Evan Stade wrote:
> this gets its own file please

Done.

https://chromiumcodereview.appspot.com/10703162/diff/15002/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/cryptohome_ui.cc:61:
SetCryptohomeBoolProperty(destination_id,
On 2012/07/19 01:53:16, Evan Stade wrote:
> all this should just be stuffed into loadTimeData. on the C++ side it looks
like
> 
>   source->AddString(...)
> 
> in CreateCryptohomeUIHTMLSource. For booleans you can either add to the
> localized_strings Dictionary directly or you could add a AddBoolean function
> that parallels AddString.

The problem with setting values to loadTimeData is that some values like
TpmIsReady are returned asynchronously. (and all Cryptohome property getters are
going to be asynchronous, crbug.com/126674)
So I prefer handling JS calls to halting page load until all asynchronous values
get ready.

Does this make sense?

[Evan Stade, 2012-07-19 22:54:41]

> Does this make sense?

yes, thanks for the explanation. I would say then that you should just have one
call from js ('chrome.send(pageLoaded)') which fires off all the async requests.

https://chromiumcodereview.appspot.com/10703162/diff/21002/chrome/browser/ui/...
File chrome/browser/ui/webui/chromeos/cryptohome_web_ui_handler.cc (right):

https://chromiumcodereview.appspot.com/10703162/diff/21002/chrome/browser/ui/...
chrome/browser/ui/webui/chromeos/cryptohome_web_ui_handler.cc:94:
SetCryptohomeStringProperty(destination_id,
how about:

void CryptohomeWebUIHandler::SetCryptohomeProperty(
    const std::string& destination_id,
    const base::Value& value) {
  web_ui()->CallJavascriptFunction(
      "SetCryptohomeProperty",
      base::StringValue(destination_id),
      value);
}

it's pretty easy for callers to wrap the C++ type with
base::FundamentalValue(value) or base::StringValue(value)

[hashimoto, 2012-07-20 10:54:28]

PTAL

http://codereview.chromium.org/10703162/diff/21002/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/cryptohome_web_ui_handler.cc (right):

http://codereview.chromium.org/10703162/diff/21002/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/cryptohome_web_ui_handler.cc:94:
SetCryptohomeStringProperty(destination_id,
On 2012/07/19 22:54:41, Evan Stade wrote:
> how about:
> 
> void CryptohomeWebUIHandler::SetCryptohomeProperty(
>     const std::string& destination_id,
>     const base::Value& value) {
>   web_ui()->CallJavascriptFunction(
>       "SetCryptohomeProperty",
>       base::StringValue(destination_id),
>       value);
> }
> 
> it's pretty easy for callers to wrap the C++ type with
> base::FundamentalValue(value) or base::StringValue(value)

Done.

http://codereview.chromium.org/10703162/diff/32001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/cryptohome_web_ui_handler.cc (right):

http://codereview.chromium.org/10703162/diff/32001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/cryptohome_web_ui_handler.cc:34:
base::FundamentalValue is_mounted(cryptohome_library->IsMounted());
Local variables like this |is_mounted| are needed to suppress clang's error
"C++98 requires an accessible copy constructor for class
'base::FundamentalValue' when binding a reference to a temporary"

[Evan Stade, 2012-07-20 18:29:45]

lgtm

http://codereview.chromium.org/10703162/diff/32001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/cryptohome_web_ui_handler.cc (right):

http://codereview.chromium.org/10703162/diff/32001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/cryptohome_web_ui_handler.cc:34:
base::FundamentalValue is_mounted(cryptohome_library->IsMounted());
On 2012/07/20 10:54:28, hashimoto wrote:
> Local variables like this |is_mounted| are needed to suppress clang's error
> "C++98 requires an accessible copy constructor for class
> 'base::FundamentalValue' when binding a reference to a temporary"

oh, that sucks.

http://codereview.chromium.org/10703162/diff/32001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/cryptohome_web_ui_handler.cc:49: std::string
token_name;
declare inside the if block

[hashimoto, 2012-07-23 04:33:24]

http://codereview.chromium.org/10703162/diff/32001/chrome/browser/ui/webui/ch...
File chrome/browser/ui/webui/chromeos/cryptohome_web_ui_handler.cc (right):

http://codereview.chromium.org/10703162/diff/32001/chrome/browser/ui/webui/ch...
chrome/browser/ui/webui/chromeos/cryptohome_web_ui_handler.cc:49: std::string
token_name;
On 2012/07/20 18:29:45, Evan Stade wrote:
> declare inside the if block

Done.

[commit-bot: I haz the power, 2012-07-23 06:48:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/10703162/22037

[commit-bot: I haz the power, 2012-07-23 07:41:41]

Try job failure for 10703162-22037 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...