Unify var tracking between webkit and the proxy.

Unify var tracking between webkit and the proxy.

This replaces the var tracking in the proxy with the var tracking in the
shared_impl that's used by the implementation. It adds a new ProxyObjectVar
to be the proxied plugin analog of NPObjectVar in the impl. This new object
just keeps track of the host data.

The tricky part is to make the var tracker able to do all the crazy messaging.
This adds some virtual functions to the shared var tracker that we override
in the plugin in PluginVarTracker.

This removes the calls to the GetLiveObjectsForInstance in the var deprecated
test. It turns out this function really can't be implemented properly in the
proxy, and I don't know why it even worked before. A Release() call posts a
non-nestable task so the object isn't released until later. So to implement
the proxy for GetLiveObjectsForInstance we would also need to post a
non-nestable task. But when the test runs we're getting called from within
the plugin, so blocking on a non-nestable task deadlocks. So I just gave up
and deleted the parts of the test that uses it.

TEST=included
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=96094

[brettw, 2011-08-07 19:05:36]

Kind of complicated, sorry. Good thing I wrote those extra unit tests last week,
they saved my butt.

[dmichael (off chromium), 2011-08-08 22:13:59]

mostly LGTM, but I have some comments and there are a couple of places where the
IDType for Var seems wrong :-/

I'm wondering if it might be best to get rid of that ID Type stuff altogether,
or is it truly necessary in the host side?

http://codereview.chromium.org/7578001/diff/9052/ppapi/proxy/plugin_var_seria...
File ppapi/proxy/plugin_var_serialization_rules.cc (right):

http://codereview.chromium.org/7578001/diff/9052/ppapi/proxy/plugin_var_seria...
ppapi/proxy/plugin_var_serialization_rules.cc:47: return
StringVar::StringToPPVar(0, *str_val);
The 0 for module is just because Module is irrelevant on the plugin side, since
there's 1 per process? Could you add a comment or a separate function to make it
clearer?

http://codereview.chromium.org/7578001/diff/9052/ppapi/proxy/plugin_var_track...
File ppapi/proxy/plugin_var_tracker.h (right):

http://codereview.chromium.org/7578001/diff/9052/ppapi/proxy/plugin_var_track...
ppapi/proxy/plugin_var_tracker.h:69: virtual int32 AddVarInternal(::ppapi::Var*
var, bool take_ref) OVERRIDE;
optional suggestion: I almost always prefer enums over bool parameters, to make
the call-site more readable and harder to mess up.

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/id_assignm...
File ppapi/shared_impl/id_assignment.cc (right):

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/id_assignm...
ppapi/shared_impl/id_assignment.cc:13: const int32 kMaxPPIdType =
std::numeric_limits<int32>::max() >> kPPIdTypeBits;
Is this really the MaxPPIdType, or is it the MaxPPId? (i.e., the maximum valid
value for an ID?)

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/id_assignm...
File ppapi/shared_impl/id_assignment.h (right):

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/id_assignm...
ppapi/shared_impl/id_assignment.h:19: PP_ID_TYPE_COUNT
Can you comment a little about COUNT not being a real type, and needing to be at
the end? At a glance, I thought 'hey, 5 things don't fit in 2 bits, this is a
bug'

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/id_assignm...
ppapi/shared_impl/id_assignment.h:24: extern const int32 kMaxPPIdType;
Why does this assume int32, but the functions are templates?

It would be simpler if the functions all used int32 explicitly, if that's the
only type for which they are instantiated...

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/id_assignm...
ppapi/shared_impl/id_assignment.h:27: return (value << kPPIdTypeBits) |
static_cast<T>(type);
So you use the least-significant bits for the type? Interesting; I somehow
assumed the opposite. It would be good to have a comment somewhere on the
overall approach and why you're doing this.

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/id_assignm...
ppapi/shared_impl/id_assignment.h:31: // 0 is a valid resource.
Apparently, 0 is also a valid Var, Instance, and Module, too? Or should it only
return true if type == PP_ID_TYPE_RESOURCE when id is 0?

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/var_tracke...
File ppapi/shared_impl/var_tracker.cc (right):

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/var_tracke...
ppapi/shared_impl/var_tracker.cc:51: DLOG_IF(ERROR, !CheckIdType(var_id,
PP_ID_TYPE_VAR))
I'm having trouble figuring out where the var type bits actually gets injected
in to the var ID...? At least in the plugin side, I'm not seeing it. In the ID
assignment portion of this class, it looks like a straight increment from the
last ID, and the IDs count from 0.

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/var_tracke...
ppapi/shared_impl/var_tracker.cc:116: // If the plugin manages to create 1B
strings...
This is splitting hairs, but I think it's more like 500M ;-)
max() = 0111...1 ~= 2B 
max() >> 2 = 0001...1 ~= 500M
Are you using the highest order 2 bits for the type? If so, you maybe should be
using unsigned?

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/var_tracke...
ppapi/shared_impl/var_tracker.cc:120: // Add the resource with plugin use-count
1.
Update comment to account for take_ref? Or just remove? It's kind of
self-evident.

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/var_tracker.h
File ppapi/shared_impl/var_tracker.h (right):

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/var_tracke...
ppapi/shared_impl/var_tracker.h:51: // returns true and does nothing for
non-refcounted type vars.
Maybe mention the Var is deleted if ref count goes to 0?

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/var_tracke...
ppapi/shared_impl/var_tracker.h:98: // there are no proxy objects.
optional suggest: You could have a member on this base class indicating if
you're in the host or not, just to do DCHECKs in these methods that only do
stuff in the plugin process.

http://codereview.chromium.org/7578001/diff/9052/ppapi/tests/test_var_depreca...
File ppapi/tests/test_var_deprecated.cc (right):

http://codereview.chromium.org/7578001/diff/9052/ppapi/tests/test_var_depreca...
ppapi/tests/test_var_deprecated.cc:355: // Get a valid property/method when the
exception is set returns false.
nit: Get->Getting

[brettw, 2011-08-09 17:08:09]

Addressed everything, a few responses below.

http://codereview.chromium.org/7578001/diff/9052/ppapi/proxy/plugin_var_seria...
File ppapi/proxy/plugin_var_serialization_rules.cc (right):

http://codereview.chromium.org/7578001/diff/9052/ppapi/proxy/plugin_var_seria...
ppapi/proxy/plugin_var_serialization_rules.cc:47: return
StringVar::StringToPPVar(0, *str_val);
Correct, I've been wanting to force people do to the right thing, so I filed a
bug (we should have some global ID we use for cases like this).

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/id_assignm...
File ppapi/shared_impl/id_assignment.h (right):

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/id_assignm...
ppapi/shared_impl/id_assignment.h:24: extern const int32 kMaxPPIdType;
I think this is because this is old code and resources used to be 64 bits. I
think I want to make the var IDs 64 bits to match PP_Var, so I'm inclined to
leave the templates for now.

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/var_tracke...
File ppapi/shared_impl/var_tracker.cc (right):

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/var_tracke...
ppapi/shared_impl/var_tracker.cc:51: DLOG_IF(ERROR, !CheckIdType(var_id,
PP_ID_TYPE_VAR))
VarTracker::AddVarInternal calls MakeTypedId.

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/var_tracker.h
File ppapi/shared_impl/var_tracker.h (right):

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/var_tracke...
ppapi/shared_impl/var_tracker.h:98: // there are no proxy objects.
The implementation of this function is actually NOTREACHED().

[dmichael (off chromium), 2011-08-09 19:16:48]

LGTM

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/var_tracker.h
File ppapi/shared_impl/var_tracker.h (right):

http://codereview.chromium.org/7578001/diff/9052/ppapi/shared_impl/var_tracke...
ppapi/shared_impl/var_tracker.h:98: // there are no proxy objects.
On 2011/08/09 17:08:09, brettw wrote:
> The implementation of this function is actually NOTREACHED().
Missed that; perfect, thanks.