Remove Pickle::WriteSize() now that it has no remaining callers.

Remove Pickle::WriteSize() now that it has no remaining callers. Also rename
Pickle::WriteLong() to WriteLongUsingDangerousNonPortableLessPersistableForm()
and add a strongly-worded comment against its use; it is used only for IPC
which is safe. (These methods write variable amounts of data to pickles,
depending on architecture, and aren't safe if pickles are persisted.)

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

[Mike Mammarella, 2012-03-09 20:58:58]

On 2012/03/08 02:25:40, Mike Mammarella wrote:

jar: ping

[jar (doing other things), 2012-03-09 22:13:36]

https://chromiumcodereview.appspot.com/9641005/diff/1/base/pickle.h
File base/pickle.h (right):

https://chromiumcodereview.appspot.com/9641005/diff/1/base/pickle.h#newcode105
base/pickle.h:105: // between dissimilar systems, YOUR PICKLES WILL HAVE GONE
BAD.
IMO, you have discovered a problem.... and it is a bad problem for anyone that
persists data using pickles.  

Rather than having a comment (resetting the mouse-trap, so that it will catch
the next victim, and cost us all), how abut crafting some code to handle this
issue more directly.

Can we get rid of this API?  How many users are there?  Can you migrate them to
a more stable API??

Jim

[Mike Mammarella, 2012-03-10 00:24:42]

https://chromiumcodereview.appspot.com/9641005/diff/1/base/pickle.h
File base/pickle.h (right):

https://chromiumcodereview.appspot.com/9641005/diff/1/base/pickle.h#newcode105
base/pickle.h:105: // between dissimilar systems, YOUR PICKLES WILL HAVE GONE
BAD.
On 2012/03/09 22:13:36, jar wrote:
> IMO, you have discovered a problem.... and it is a bad problem for anyone that
> persists data using pickles.  
> 
> Rather than having a comment (resetting the mouse-trap, so that it will catch
> the next victim, and cost us all), how abut crafting some code to handle this
> issue more directly.
> 
> Can we get rid of this API?  How many users are there?  Can you migrate them
to
> a more stable API??
> 
> Jim

Looks like WriteSize() might be feasible to remove, as it's not used in too many
places. (As long as all such places don't persist data, we can just change
them.) WriteLong() is used in the IPC code, which we should be able to change
safely, but it's all over the place. I'll look into it.

That said, this warning can only help compared to doing nothing, and I don't
work full-time on Chrome, so it could take a while for me to investigate this.
I'd like to add the warning for now.

[Mike Mammarella, 2012-03-22 20:12:47]

ping

[jar (doing other things), 2012-03-22 21:08:57]

https://chromiumcodereview.appspot.com/9641005/diff/6001/base/pickle.h
File base/pickle.h (right):

https://chromiumcodereview.appspot.com/9641005/diff/6001/base/pickle.h#newcod...
base/pickle.h:198: // between dissimilar systems, YOUR PICKLES WILL HAVE GONE
BAD.
How about changing the name to something that clarifies that the user should not
be using it?  How about:

WriteLongUsingDangerousNonPortableLessPersistableForm()

This would avoid "using it by accident," and alert reviewers about the issue
that is best(?) avoided.

[Mike Mammarella, 2012-03-22 21:30:52]

https://chromiumcodereview.appspot.com/9641005/diff/6001/base/pickle.h
File base/pickle.h (right):

https://chromiumcodereview.appspot.com/9641005/diff/6001/base/pickle.h#newcod...
base/pickle.h:198: // between dissimilar systems, YOUR PICKLES WILL HAVE GONE
BAD.
On 2012/03/22 21:08:57, jar wrote:
> How about changing the name to something that clarifies that the user should
not
> be using it?  How about:
> 
> WriteLongUsingDangerousNonPortableLessPersistableForm()
> 
> This would avoid "using it by accident," and alert reviewers about the issue
> that is best(?) avoided.

I took a look and it turns out that WriteLong() is only used in one place, for
IPC - it just caused tons of compiler errors when I removed it the first time so
I had assumed it was all over the place. I've renamed the method and updated the
use in IPC, since I assume we don't want to add unnecessary overhead there (e.g.
by using 64 bits on 32-bit platforms).

[jar (doing other things), 2012-03-22 21:46:50]

lgtm

[Mike Mammarella, 2012-03-22 21:52:23]

+jam for ipc

[jam, 2012-03-22 21:54:01]

lgtm

[commit-bot: I haz the power, 2012-03-22 21:55:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mdm@chromium.org/9641005/16001

[commit-bot: I haz the power, 2012-03-23 00:03:11]

Change committed as 128347