Clean up PnaclTranslationCache interface

Clean up PnaclTranslationCache interface

This CL augments PnaclTranslationCache's public interface into a state
where it can be used by the PnaclHost. In particular, it adds GetKey
which generates a cache key from a cache_info object, and it switches
to using net::DrainableIOBuffer to transfer nexe data, which both
simplifies the implementation and reduces the number of times the data
needs to be copied.

R=jvoung@chromium.org
BUG= https://code.google.com/p/nativeclient/issues/detail?id=3372
TEST= unit_tests --gtest_filter=PnaclTranslationCache*

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214755
Reverted in http://src.chromium.org/viewvc/chrome?view=rev&revision=214762

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

[Derek Schuff, 2013-07-30 21:50:41]

oops, code review will be faster if i send the mail

[jvoung (off chromium), 2013-07-30 22:51:39]

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
File chrome/browser/nacl_host/pnacl_translation_cache.cc (right):

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_translation_cache.cc:359: StoreNexe(key,
nexe_data, CompletionCallback());
Does this version of StoreNexe need to remain?

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_translation_cache.cc:410: retval += ";" +
IntToString(info.abi_version) + ";" +
Could label the fields, so that it's easier to tell which part is which?

Should the non/less-user-controlled parts like the abi version and opt level
come first?

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_translation_cache.cc:414: // here means we can
keep the format stable.
There's strftime, but then you'd have to convert the base::Time to "struct tm". 
Otherwise, yeah you'd think there was a base method...

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_translation_cache.cc:421:
IntToString(exploded.millisecond) + ";";
Could label the time UTC.

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
File chrome/browser/nacl_host/pnacl_translation_cache.h (right):

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_translation_cache.h:34: NexeCallback;
Could this be named something like GetNexeCallback so that it seems less like it
could be used by the Stores?

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
File chrome/browser/nacl_host/pnacl_translation_cache_unittest.cc (right):

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_translation_cache_unittest.cc:64: new
net::DrainableIOBuffer(new net::StringIOBuffer(nexe), nexe.size()));
Does DrainableIOBuffer take ownership of the thing it wraps, or not?

[Derek Schuff, 2013-07-31 00:04:16]

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
File chrome/browser/nacl_host/pnacl_translation_cache.cc (right):

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_translation_cache.cc:359: StoreNexe(key,
nexe_data, CompletionCallback());
On 2013/07/30 22:51:39, jvoung (cr) wrote:
> Does this version of StoreNexe need to remain?
This is the one my current version of the code actually uses, but one of my
current TODOs is to keep the translation pending until the callback returns. so
it's still here for now.

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_translation_cache.cc:410: retval += ";" +
IntToString(info.abi_version) + ";" +
On 2013/07/30 22:51:39, jvoung (cr) wrote:
> Could label the fields, so that it's easier to tell which part is which?
> 
> Should the non/less-user-controlled parts like the abi version and opt level
> come first?

Done.

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_translation_cache.cc:414: // here means we can
keep the format stable.
On 2013/07/30 22:51:39, jvoung (cr) wrote:
> There's strftime, but then you'd have to convert the base::Time to "struct
tm". 
> Otherwise, yeah you'd think there was a base method...
Yeah in the end I decided it's just as well that we control it ourselves.

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_translation_cache.cc:421:
IntToString(exploded.millisecond) + ";";
On 2013/07/30 22:51:39, jvoung (cr) wrote:
> Could label the time UTC.

Done.

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
File chrome/browser/nacl_host/pnacl_translation_cache.h (right):

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_translation_cache.h:34: NexeCallback;
On 2013/07/30 22:51:39, jvoung (cr) wrote:
> Could this be named something like GetNexeCallback so that it seems less like
it
> could be used by the Stores?

Done.

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
File chrome/browser/nacl_host/pnacl_translation_cache_unittest.cc (right):

https://codereview.chromium.org/21075004/diff/4001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_translation_cache_unittest.cc:64: new
net::DrainableIOBuffer(new net::StringIOBuffer(nexe), nexe.size()));
On 2013/07/30 22:51:39, jvoung (cr) wrote:
> Does DrainableIOBuffer take ownership of the thing it wraps, or not?
It keeps a scoped_refptr to what it wraps, so when it gets deleted, the internal
one will get deleted too.

[jvoung (off chromium), 2013-07-31 00:58:38]

LGTM

https://codereview.chromium.org/21075004/diff/16001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/pnacl_translation_cache_unittest.cc (right):

https://codereview.chromium.org/21075004/diff/16001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/pnacl_translation_cache_unittest.cc:140:
PnaclTranslationCache::GetKey(info));
Could also check that other schemes like chrome-extension:// URLs are okay.

https://codereview.chromium.org/21075004/diff/16001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/pnacl_translation_cache_unittest.cc:186: // backend.
Find out what the 'offset' field means, and if it can ever require
Is this TODO still relevant w/ change to DrainableIOBuffer?

[Derek Schuff, 2013-07-31 05:38:33]

https://codereview.chromium.org/21075004/diff/16001/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/pnacl_translation_cache_unittest.cc (right):

https://codereview.chromium.org/21075004/diff/16001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/pnacl_translation_cache_unittest.cc:140:
PnaclTranslationCache::GetKey(info));
On 2013/07/31 00:58:38, jvoung (cr) wrote:
> Could also check that other schemes like chrome-extension:// URLs are okay.

Done.

https://codereview.chromium.org/21075004/diff/16001/chrome/browser/nacl_host/...
chrome/browser/nacl_host/pnacl_translation_cache_unittest.cc:186: // backend.
Find out what the 'offset' field means, and if it can ever require
On 2013/07/31 00:58:38, jvoung (cr) wrote:
> Is this TODO still relevant w/ change to DrainableIOBuffer?
Yes, the part that interacts with the disk_cache interface is the same. the
change really just affects the outward-facing API. although i ended up making it
so the code should work either way, so maybe ill just get rid of this comment.

[commit-bot: I haz the power, 2013-07-31 05:43:41]

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

[commit-bot: I haz the power, 2013-07-31 06:13:07]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2013-07-31 15:18:27]

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

[commit-bot: I haz the power, 2013-07-31 16:17:06]

Change committed as 214755

[Derek Schuff, 2013-07-31 18:12:12]

On 2013/07/31 16:17:06, I haz the power (commit-bot) wrote:
> Change committed as 214755

This change was reverted because some systems (i.e., the linux trybot vs the
buildbot) seemed to disagree on what the exploded representation of a null
base::Time should be, which caused the unit test (which used some null Times) to
fail.

[Derek Schuff, 2013-07-31 18:24:39]

handling null base::Time explicitly now, PTAL

[jvoung (off chromium), 2013-07-31 18:27:05]

On 2013/07/31 18:24:39, Derek Schuff wrote:
> handling null base::Time explicitly now, PTAL

LGTM

[commit-bot: I haz the power, 2013-07-31 18:29:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dschuff@chromium.org/21075004/35002

[commit-bot: I haz the power, 2013-07-31 23:24:20]

Change committed as 214859