Var serialization-related proxy stuff. This allows vars to be sent over IPC...

Var serialization-related proxy stuff. This allows vars to be sent over IPC
and received with proper refcounting semantics.

The basic design is that the SerializedVar is the thing actually handled by
IPC. Then you use one of the many helper classes depending on whether you are
sending or receiving and what the ownership semantics are. The helper classes,
along with the VarSerialization interface makes the right thing happen for
string conversion & refcounting.

The VarSerialization class is implemented differently on the browser and plugin
side and is where the differences in refcounting & strings are handled. This
allows the rest of the code to be the same on both the plugin and browser side
and allows vars to be sent back and forth.

This patch references some files not in it like the dispatcher and various
tracker classes. This is the cleanest cut I could make for a reasonably
reviewable chunk.

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

[brettw, 2010-11-03 02:26:23]

New snap up with VarSerialization renamed VarSerializationRules to be less
confusing. I also renamed Browser* to Host* which better matches Chrome (and is
less confusing since this actually runs in the renderer process, not the browser
process).

[neb, 2010-11-03 04:41:44]

This feels too complicated, I think we really must simplify it a bit. Let's
start with SerializedVar and SerializationRules.

http://codereview.chromium.org/4096008/diff/10001/11004
File ppapi/proxy/plugin_var_serialization.h (right):

http://codereview.chromium.org/4096008/diff/10001/11004#newcode19
ppapi/proxy/plugin_var_serialization.h:19: // This class with use the given
non-owning pointer to the var tracker to
There's something wrong with this sentence.

http://codereview.chromium.org/4096008/diff/10001/11006
File ppapi/proxy/serialized_var.h (right):

http://codereview.chromium.org/4096008/diff/10001/11006#newcode37
ppapi/proxy/serialized_var.h:37: // This is sadly super complicated. The IPC
must all use the same type and it
Can you reword this paragraph a bit? It's confusing.

http://codereview.chromium.org/4096008/diff/10001/11007
File ppapi/proxy/var_serialization.h (right):

http://codereview.chromium.org/4096008/diff/10001/11007#newcode28
ppapi/proxy/var_serialization.h:28: // a string, str_val will beuntouched and
may be NULL.
beuntouched -> be untouched

http://codereview.chromium.org/4096008/diff/29001/30006
File ppapi/proxy/serialized_var.h (right):

http://codereview.chromium.org/4096008/diff/29001/30006#newcode66
ppapi/proxy/serialized_var.h:66: class SerializedVar {
I have a couple of questions about this class.

1. I am not convinced that we need the inner class. I see that the message sent
by address, and is de-pickled in Dispatch() and then passed by reference (hence
your const acrobatics). At what point was this copying happening?

2. If we decide to use an inner class, we should use shared pointer directly.
And if we have to use a shared ptr, can we use ref_ptr instead if linked_ptr? We
use ref_ptrs everywhere else, they are thread safe, and since we control the
class we don't need refcounting to be external.

http://codereview.chromium.org/4096008/diff/29001/30007
File ppapi/proxy/var_serialization_rules.h (right):

http://codereview.chromium.org/4096008/diff/29001/30007#newcode18
ppapi/proxy/var_serialization_rules.h:18: class VarSerializationRules {
Again, maybe the serialization rules are not the correct place to define the
interface. You'll see that the serialization rules are exactly the same, except
that

1. plugin uses var_tracker_->GetString() instead of VarToString()
2. plugin uses var_tracker_->MakeString() instead of
var_interface_->VarFromUtf8()
3. plugin uses var_tracker_->Release() instead of var_interface_->Release()
4. plugin uses var_tracker_->ReceiveObjectPassRef() instead of
var_interface_->AddRef()

I think defining this interface would allow you to reuse all the logic, and will
also be easier to follow. SerializationRules are... hard to follow :(.

[brettw, 2010-11-03 05:44:24]

http://codereview.chromium.org/4096008/diff/29001/30006
File ppapi/proxy/serialized_var.h (right):

http://codereview.chromium.org/4096008/diff/29001/30006#newcode66
ppapi/proxy/serialized_var.h:66: class SerializedVar {
On 2010/11/03 04:41:44, neb wrote:
> I have a couple of questions about this class.
> 
> 1. I am not convinced that we need the inner class. I see that the message
sent
> by address, and is de-pickled in Dispatch() and then passed by reference
(hence
> your const acrobatics). At what point was this copying happening?

The pickeling system does copying internally. I discovered this the hard way
when I tried to prevent copying but the compile fails with a 50 lines of
template errors.
 
> 2. If we decide to use an inner class, we should use shared pointer directly.
> And if we have to use a shared ptr, can we use ref_ptr instead if linked_ptr?
We
> use ref_ptrs everywhere else, they are thread safe, and since we control the
> class we don't need refcounting to be external.

That's fine.

http://codereview.chromium.org/4096008/diff/29001/30007
File ppapi/proxy/var_serialization_rules.h (right):

http://codereview.chromium.org/4096008/diff/29001/30007#newcode18
ppapi/proxy/var_serialization_rules.h:18: class VarSerializationRules {
On 2010/11/03 04:41:44, neb wrote:
> Again, maybe the serialization rules are not the correct place to define the
> interface. You'll see that the serialization rules are exactly the same,
except
> that
> 
> 1. plugin uses var_tracker_->GetString() instead of VarToString()
> 2. plugin uses var_tracker_->MakeString() instead of
> var_interface_->VarFromUtf8()
> 3. plugin uses var_tracker_->Release() instead of var_interface_->Release()
> 4. plugin uses var_tracker_->ReceiveObjectPassRef() instead of
> var_interface_->AddRef()
> 
> I think defining this interface would allow you to reuse all the logic, and
will
> also be easier to follow. SerializationRules are... hard to follow :(.

I don't understand what you're saying here. You're saying that the var
serialization rules isn't the right place to define "the interface" and then "if
we defined this interface...". This class is entirely about defining the
differences between the plugin and the browser and exposing them in an interface
that's the same for both ends. Where would you put this logic?

[brettw, 2010-11-03 16:43:32]

New snap up.

[brettw, 2010-11-04 20:55:09]

New snap up with refptr.

[neb, 2010-11-05 21:13:50]

I still think that 1200 lines of code for this is a bit excessive. Let's talk
about it on Monday.

However, I think it is functionally correct, and a different implementation
won't change how this is used, so I'm perfectly fine with you checking it in and
then we can iterate on it later.