Use a better registration suffix that will always be unique while respecting the MSDN rules (2nd tr…

Use a better registration suffix that will always be unique while respecting the MSDN rules (2nd try). 

The suffix will now be the base32 encoding of the md5 hash of the user's username. This is always unique and can be determined many times deterministically without I/O access (as opposed to other methods we considered). 

It respects all MSDN constraints for properties onto which it is appended. 

This replaces the prior-style username suffixes for new installs. 

Old-style suffixes (i.e. unsuffixed and username suffixed) will however be kept as is through future updates. 

Design doc: https://docs.google.com/a/chromium.org/document/d/1qmcV3uYBh3JwvXhYkI7asg0nN7KfVMWVOzND4p0jQ3E/edit 

Note: This is the continuation of http://codereview.chromium.org/10617002/ which was reverted in https://chromiumcodereview.appspot.com/10667006/

BUG=133810, 133173
TEST= http://goo.gl/ZZ7gE 
installer_util_unittests.exe --gtest_filter=ShellUtilTest.*


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

[gab, 2012-06-26 22:01:31]

PTAL.

Fixed problems with the first CL (the DCHECK hit was because I was assuming
Chrome was installed... which it isn't for tests, Canary, etc.).

Note: I noticed that upgrading the appid means the shortcut will lose Metro
properties (icon only, i.e. it still launches in Metro). Those will be back up
on the next Chrome first run when "MigrateChromiumShortcuts()" is called.

I was going to call this from setup, but I think the reason its been done in
first run before is that Chrome can be currently running on update and we don't
want to mess with the shortcut at that point (I could potentially only update
the Start Screen shortcut, but I'm not sure if the behavior is defined if the
shortcut is updated while Chrome Metro is running...).

So unless this bothers anyone heavily (i.e. that affected users on dev/beta will
see Chrome without the Metro icon once). I would rather not bother with this.

Cheers,
Gab

[robertshield, 2012-06-27 02:22:29]

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:539:
browser_entry_suffix.compare(old_style_suffix) == 0)) {
on uninstall, should we not just unconditionally attempt to delete all relevant
registry keys? why bother checking suffixes when we could instead just always
try to delete the suffixed and unsuffixed versions (not caring if the deletion
attempts fail)?

[gab, 2012-06-27 03:56:56]

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:539:
browser_entry_suffix.compare(old_style_suffix) == 0)) {
On 2012/06/27 02:22:29, robertshield wrote:
> on uninstall, should we not just unconditionally attempt to delete all
relevant
> registry keys? why bother checking suffixes when we could instead just always
> try to delete the suffixed and unsuffixed versions (not caring if the deletion
> attempts fail)?

This would be more work in most cases, but I agree it would make the uninstall
logic simpler.

i.e. for user-level uninstall:
1) new-style suffixed entries
2) old-style suffixed entries
3) unsuffixed entries (in HKCU only if !remove_all)

[grt (UTC plus 2), 2012-06-27 15:47:32]

drive-by

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:539:
browser_entry_suffix.compare(old_style_suffix) == 0)) {
nit: i think "browser_entry_suffix == old_style_suffix" is more readable than
.compare() == 0

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:823: suffix.compare(old_style_suffix) != 0)
{
same nit as above: "suffix != old_style_suffix" is more readable

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:102: static string16 new_style_suffix;
this isn't threadsafe and it adds an exit-time dtor.  if you don't care about
the former, use CR_DEFINE_STATIC_LOCAL.  otherwise, use a leaky lazy instance.

why not do the caching at the level of GetNewUserSpecificRegistrySuffix?  i
don't see why this caller alone should benefit from not recomputing it.  you
could do something clean for the suffix that would allow it to be cached and for
the cache to be overridden in tests if that was helpful.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:770: // Sets |suffix| to a 27 characters
string that is specific to this user on
characters -> character

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:771: // this machine, on user-level
installs, preferably.
i don't understand "on user-level installs, preferably."  what does that mean?

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:792: string16 old_suffix;
why not use the caller-provided string here rather than introducing a new one?

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1098: (tested_suffix.clear(),
ugh, comma.  how about just:

!QuickIsChromeRegistered(dist, chrome_exe, tested_suffix.erase(),
CONFIRM_PROGID_REGISTRATION)

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1572: bool
ShellUtil::GetNewUserSpecificRegistrySuffix(string16* suffix) {
I'm not in love with "New" and "Old" in the method names.  On the plus side:
it's a bit easier for a caller to know which one to use.  On the minus side:
it's opaque.  What do you think about at least removing "New" from this one?

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1588: suffix->reserve(27);
27 -> base32_md5.size() + 1

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1595: // meant to be added to all
registration that may conflict with another
this comment is incorrect, no?  this suffix shouldn't be used to disambiguate,
it should only be used for "legacy" installs.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1611: string16
ShellUtil::MD5DigestToBase32(const base::MD5Digest& digest) {
why is this function needed?  GetNewUserSpecificRegistrySuffix does its DCHECK,
so i don't see the added value of it.  removing it gets rid of the #include
"base/md5.h" from the header, which would be a good thing IMO.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1613: string16
encoding(ByteArrayToBase32(digest.a, 16));
16 -> arraysize(digest.a)

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1620: string16
ShellUtil::ByteArrayToBase32(const unsigned char bytes[], int size) {
const unsigned char bytes[] -> const uint8* bytes
style guide says to use size_t for "a count of objects or bytes in memory",
which describes |size|.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1621: static char const kEncoding[] =
"ABCDEFGHIJKLMNOPQRSTUVWXYZ234567";
char const -> const char

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1631: }
else if (size >= std::numeric_limits<size_t>::max() / 8) {
  // error: the calculation of encoded_length would overflow.
}

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1635: const int kEncodedLength = (size * 8 +
4) / 5;
kEncodedLength -> encoded_length (it isn't a compile-time constant)

[gab, 2012-07-03 22:36:06]

Addressed all comments, PTAL.

Cheers,
Gab

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:539:
browser_entry_suffix.compare(old_style_suffix) == 0)) {
On 2012/06/27 15:47:32, grt wrote:
> nit: i think "browser_entry_suffix == old_style_suffix" is more readable than
> .compare() == 0

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:539:
browser_entry_suffix.compare(old_style_suffix) == 0)) {
On 2012/06/27 02:22:29, robertshield wrote:
> on uninstall, should we not just unconditionally attempt to delete all
relevant
> registry keys? why bother checking suffixes when we could instead just always
> try to delete the suffixed and unsuffixed versions (not caring if the deletion
> attempts fail)?

I removed this to always suffix with the passed in appid as that makes more
sense.

I however left the logic on line 823 as it only requires a small check (instead
of deleting all entries for all possible suffixes without checking).

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:823: suffix.compare(old_style_suffix) != 0)
{
On 2012/06/27 15:47:32, grt wrote:
> same nit as above: "suffix != old_style_suffix" is more readable

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:102: static string16 new_style_suffix;
On 2012/06/27 15:47:32, grt wrote:
> this isn't threadsafe and it adds an exit-time dtor.  if you don't care about
> the former, use CR_DEFINE_STATIC_LOCAL.  otherwise, use a leaky lazy instance.
> 
> why not do the caching at the level of GetNewUserSpecificRegistrySuffix?  i
> don't see why this caller alone should benefit from not recomputing it.  you
> could do something clean for the suffix that would allow it to be cached and
for
> the cache to be overridden in tests if that was helpful.

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:770: // Sets |suffix| to a 27 characters
string that is specific to this user on
On 2012/06/27 15:47:32, grt wrote:
> characters -> character

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:771: // this machine, on user-level
installs, preferably.
On 2012/06/27 15:47:32, grt wrote:
> i don't understand "on user-level installs, preferably."  what does that mean?

I meant that this is not the only outcome, but the one we "prefer". I agree it's
confusing, removed as the rest of the comment already details this out.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:792: string16 old_suffix;
On 2012/06/27 15:47:32, grt wrote:
> why not use the caller-provided string here rather than introducing a new one?

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:792: string16 old_suffix;
On 2012/06/27 15:47:32, grt wrote:
> why not use the caller-provided string here rather than introducing a new one?

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1098: (tested_suffix.clear(),
On 2012/06/27 15:47:32, grt wrote:
> ugh, comma.  how about just:
> 
> !QuickIsChromeRegistered(dist, chrome_exe, tested_suffix.erase(),
> CONFIRM_PROGID_REGISTRATION)

Oh nice, erase()! I had looked at .clear(), but it returns void() :(... Thought
I was clever with my comma operator, but that's even better :)!

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1572: bool
ShellUtil::GetNewUserSpecificRegistrySuffix(string16* suffix) {
On 2012/06/27 15:47:32, grt wrote:
> I'm not in love with "New" and "Old" in the method names.  On the plus side:
> it's a bit easier for a caller to know which one to use.  On the minus side:
> it's opaque.  What do you think about at least removing "New" from this one?

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1588: suffix->reserve(27);
On 2012/06/27 15:47:32, grt wrote:
> 27 -> base32_md5.size() + 1

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1595: // meant to be added to all
registration that may conflict with another
On 2012/06/27 15:47:32, grt wrote:
> this comment is incorrect, no?  this suffix shouldn't be used to disambiguate,
> it should only be used for "legacy" installs.

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1611: string16
ShellUtil::MD5DigestToBase32(const base::MD5Digest& digest) {
On 2012/06/27 15:47:32, grt wrote:
> why is this function needed?  GetNewUserSpecificRegistrySuffix does its
DCHECK,
> so i don't see the added value of it.  removing it gets rid of the #include
> "base/md5.h" from the header, which would be a good thing IMO.

Good point, moved the very little bit logic remaining in this function to
GetUserSpecificRegistrySuffix().

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1613: string16
encoding(ByteArrayToBase32(digest.a, 16));
On 2012/06/27 15:47:32, grt wrote:
> 16 -> arraysize(digest.a)

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1620: string16
ShellUtil::ByteArrayToBase32(const unsigned char bytes[], int size) {
On 2012/06/27 15:47:32, grt wrote:
> const unsigned char bytes[] -> const uint8* bytes
> style guide says to use size_t for "a count of objects or bytes in memory",
> which describes |size|.

I thought style-guide also said to avoid using size_t, but ok.

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1621: static char const kEncoding[] =
"ABCDEFGHIJKLMNOPQRSTUVWXYZ234567";
On 2012/06/27 15:47:32, grt wrote:
> char const -> const char

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1621: static char const kEncoding[] =
"ABCDEFGHIJKLMNOPQRSTUVWXYZ234567";
On 2012/06/27 15:47:32, grt wrote:
> char const -> const char

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1631: }
On 2012/06/27 15:47:32, grt wrote:
> else if (size >= std::numeric_limits<size_t>::max() / 8) {
>   // error: the calculation of encoded_length would overflow.
> }

Done.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1635: const int kEncodedLength = (size * 8 +
4) / 5;
On 2012/06/27 15:47:32, grt wrote:
> kEncodedLength -> encoded_length (it isn't a compile-time constant)

Done.

http://codereview.chromium.org/10662052/diff/24002/chrome/installer/util/shel...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/10662052/diff/24002/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1614: bool
ShellUtil::GetOldUserSpecificRegistrySuffix(string16* suffix) {
I thought of also caching this one in the UserSpecificRegistrySuffix class, but
it would require that the LazyInstance be in the unnamed namespace (which
probably means it always exists as opposed to only be created on the first call
with a local static?) (or have 2 of them, but I like that even less).

Let me know what you think.

[grt (UTC plus 2), 2012-07-05 20:00:19]

https://chromiumcodereview.appspot.com/10662052/diff/5002/chrome/installer/se...
File chrome/installer/setup/uninstall.cc (right):

https://chromiumcodereview.appspot.com/10662052/diff/5002/chrome/installer/se...
chrome/installer/setup/uninstall.cc:539:
browser_entry_suffix.compare(old_style_suffix) == 0)) {
On 2012/07/03 22:36:06, gab wrote:
> On 2012/06/27 02:22:29, robertshield wrote:
> > on uninstall, should we not just unconditionally attempt to delete all
> relevant
> > registry keys? why bother checking suffixes when we could instead just
always
> > try to delete the suffixed and unsuffixed versions (not caring if the
deletion
> > attempts fail)?
> 
> I removed this to always suffix with the passed in appid as that makes more
> sense.
> 
> I however left the logic on line 823 as it only requires a small check
(instead
> of deleting all entries for all possible suffixes without checking).

Does this mean old values will be left behind now?

https://chromiumcodereview.appspot.com/10662052/diff/5002/chrome/installer/ut...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10662052/diff/5002/chrome/installer/ut...
chrome/installer/util/shell_util.cc:1620: string16
ShellUtil::ByteArrayToBase32(const unsigned char bytes[], int size) {
On 2012/07/03 22:36:06, gab wrote:
> On 2012/06/27 15:47:32, grt wrote:
> > const unsigned char bytes[] -> const uint8* bytes
> > style guide says to use size_t for "a count of objects or bytes in memory",
> > which describes |size|.
> 
> I thought style-guide also said to avoid using size_t, but ok.

it says to avoid using unsigned types just for the sake of representing values
that shouldn't be negative (prefer int), but do use size_t for things that truly
are sizes or counts of bytes or elements in a container.

https://chromiumcodereview.appspot.com/10662052/diff/24002/chrome/installer/u...
File chrome/installer/util/shell_util.cc (right):

https://chromiumcodereview.appspot.com/10662052/diff/24002/chrome/installer/u...
chrome/installer/util/shell_util.cc:120: UserSpecificRegistrySuffix() {
style dictates not inlining like this even though it's convenient
(http://www.chromium.org/developers/coding-style#TOC-Inline-functions)

https://chromiumcodereview.appspot.com/10662052/diff/24002/chrome/installer/u...
chrome/installer/util/shell_util.cc:132: base::MD5Sum(user_name, size,
&md5_digest);
this includes the string terminator in the hash.  please either switch to size-1
or mention in the comment above that the terminator is included.

https://chromiumcodereview.appspot.com/10662052/diff/24002/chrome/installer/u...
chrome/installer/util/shell_util.cc:1614: bool
ShellUtil::GetOldUserSpecificRegistrySuffix(string16* suffix) {
On 2012/07/03 22:36:07, gab wrote:
> I thought of also caching this one in the UserSpecificRegistrySuffix class,
but
> it would require that the LazyInstance be in the unnamed namespace (which
> probably means it always exists as opposed to only be created on the first
call
> with a local static?) (or have 2 of them, but I like that even less).

It could be done with a single class/instance owned by a private helper function
in the unnamed namespace that returns the lazy-initialized value for use by the
two Getters.

> Let me know what you think.

I think it's fine to this to stay as-is.  I don't know that GetUserName is
particularly heavy-weight.

[gab, 2012-07-05 21:19:52]

Addressed all comments.

Patched code in patch set 6 and then moved it (unmodified) in patch set 7
(hopefully that helps you save time while finishing up this review :)).

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/setup/unin...
File chrome/installer/setup/uninstall.cc (right):

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/setup/unin...
chrome/installer/setup/uninstall.cc:539:
browser_entry_suffix.compare(old_style_suffix) == 0)) {
On 2012/07/05 20:00:19, grt wrote:
> On 2012/07/03 22:36:06, gab wrote:
> > On 2012/06/27 02:22:29, robertshield wrote:
> > > on uninstall, should we not just unconditionally attempt to delete all
> > relevant
> > > registry keys? why bother checking suffixes when we could instead just
> always
> > > try to delete the suffixed and unsuffixed versions (not caring if the
> deletion
> > > attempts fail)?
> > 
> > I removed this to always suffix with the passed in appid as that makes more
> > sense.
> > 
> > I however left the logic on line 823 as it only requires a small check
> (instead
> > of deleting all entries for all possible suffixes without checking).
> 
> Does this mean old values will be left behind now?

No, it means I'm using the browser_entry_suffix passed in (instead of doing some
crazy logic to bypass GetBrowserModelId() here (which I was doing up to this
patchset)).

The logic on line 823 below was kept to pass in the old suffixes when
appropriate.

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/10662052/diff/5002/chrome/installer/util/shell...
chrome/installer/util/shell_util.cc:1620: string16
ShellUtil::ByteArrayToBase32(const unsigned char bytes[], int size) {
On 2012/07/05 20:00:19, grt wrote:
> On 2012/07/03 22:36:06, gab wrote:
> > On 2012/06/27 15:47:32, grt wrote:
> > > const unsigned char bytes[] -> const uint8* bytes
> > > style guide says to use size_t for "a count of objects or bytes in
memory",
> > > which describes |size|.
> > 
> > I thought style-guide also said to avoid using size_t, but ok.
> 
> it says to avoid using unsigned types just for the sake of representing values
> that shouldn't be negative (prefer int), but do use size_t for things that
truly
> are sizes or counts of bytes or elements in a container.

Ah ok got it, thanks for clarifying :)!

http://codereview.chromium.org/10662052/diff/24002/chrome/installer/util/shel...
File chrome/installer/util/shell_util.cc (right):

http://codereview.chromium.org/10662052/diff/24002/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:120: UserSpecificRegistrySuffix() {
On 2012/07/05 20:00:19, grt wrote:
> style dictates not inlining like this even though it's convenient
> (http://www.chromium.org/developers/coding-style#TOC-Inline-functions)

Oh interesting, I didn't know the compiler interpreted this differently (i.e.
automatically inlined it when declared this way).

More C++ knowledge woohoo!

http://codereview.chromium.org/10662052/diff/24002/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:132: base::MD5Sum(user_name, size,
&md5_digest);
On 2012/07/05 20:00:19, grt wrote:
> this includes the string terminator in the hash.  please either switch to
size-1
> or mention in the comment above that the terminator is included.

Changed to sid string.

http://codereview.chromium.org/10662052/diff/24002/chrome/installer/util/shel...
chrome/installer/util/shell_util.cc:1614: bool
ShellUtil::GetOldUserSpecificRegistrySuffix(string16* suffix) {
On 2012/07/05 20:00:19, grt wrote:
> On 2012/07/03 22:36:07, gab wrote:
> > I thought of also caching this one in the UserSpecificRegistrySuffix class,
> but
> > it would require that the LazyInstance be in the unnamed namespace (which
> > probably means it always exists as opposed to only be created on the first
> call
> > with a local static?) (or have 2 of them, but I like that even less).
> 
> It could be done with a single class/instance owned by a private helper
function
> in the unnamed namespace that returns the lazy-initialized value for use by
the
> two Getters.
> 
> > Let me know what you think.
> 
> I think it's fine to this to stay as-is.  I don't know that GetUserName is
> particularly heavy-weight.

Right, GetUserName is probably very low-height (and potentially even kept in
memory by the OS). Plus now that I use the SID, the old one is no longer and
intermediate state of the new one...

[grt (UTC plus 2), 2012-07-06 13:28:56]

lgtm!

[commit-bot: I haz the power, 2012-07-06 14:35:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/10662052/15002

[commit-bot: I haz the power, 2012-07-06 15:53:21]

Change committed as 145596