Add PepperTryCatch and V8ObjectVar classes

Add PepperTryCatch and V8ObjectVar classes

This adds PepperTryCatch and V8ObjectVar classes which will be used in the
refactoring which replaces the NPAPI bindings with gin bindings for pepper.

V8ObjectVar is a fairly straightforward replacement for NPObjectVar.

PepperTryCatch (and subclasses) are used to conveniently catch and report
errors eminating from either pepper or V8. It also includes facilities for
conveniently converting between v8 objects and vars. It is essentially the
equivalent of some of the utilities in npapi_glue.h.

This also adds conversion for object vars to the V8VarConverter and some
unimplemented functions to the HostVarTracker to allow these new files to 
compile.

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

[dmichael (off chromium), 2014-07-30 19:34:14]

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:668:
container_->element().document().frame()->mainWorldScriptContext();
The frame can be NULL in rare cases, so please check that and return a null
Context in that case.

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
File content/renderer/pepper/pepper_try_catch.cc (right):

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.cc:122: if (!GetContext().IsEmpty())
This should never be empty, right? Maybe you can just:
  GetContext()->Exit();
unconditionally?

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.cc:130: if (exception_) {
Is this so you can allow a user of this class to just eat the exception?

If so, I think the documentation in the header should say they can pass a NULL
var for that.

OTOH, I actually think it would be better to just do it unconditionally, and
force the user to pass a valid var. That way, they at least have to think about
it and be explicit if they're ignoring it. (Just seeing NULL at the call site is
easier to overlook than an unused out-param)

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
File content/renderer/pepper/pepper_try_catch.h (right):

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.h:58: PP_Var exception_;
It looks like using scoped_refptr<StringVar> might be a little cleaner and
protect against ref-count mistakes?

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.h:69: PP_Var* exception);
ScopedPPVar*, maybe?

Otherwise the user of PepperTryCatchVar has to be careful to take ownership of
the string's ref-count when an exception is set.

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.h:78: // We typically don't have a
HandleScope when using PepperTryCatchVar.
Maybe a clearer wording would be:
// Code which uses PepperTryCatchVar doesn't typically have a HandleScope, so we
make one
// for them. Note both HandleScope and PepperTryCatch are always stack
variables.
...or something.

(Alternately you could just make clients make the HandleScope themselves. This
feels slightly weird to me, but I can see why you did it.)

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
File content/renderer/pepper/v8object_var.cc (right):

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/v8object_var.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
nit: "(c) 2011"->"2014" (header too)

[raymes, 2014-08-05 06:49:35]

Thanks for your comments! I was OOO for a couple of days so I should
get back to getting this landed tomorrow.


On Thu, Jul 31, 2014 at 5:34 AM,  <dmichael@chromium.org> wrote:
>
>
https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
> File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):
>
>
https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
> content/renderer/pepper/pepper_plugin_instance_impl.cc:668:
> container_->element().document().frame()->mainWorldScriptContext();
> The frame can be NULL in rare cases, so please check that and return a
> null Context in that case.
>
>
https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
> File content/renderer/pepper/pepper_try_catch.cc (right):
>
>
https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
> content/renderer/pepper/pepper_try_catch.cc:122: if
> (!GetContext().IsEmpty())
> This should never be empty, right? Maybe you can just:
>   GetContext()->Exit();
> unconditionally?
>
>
https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
> content/renderer/pepper/pepper_try_catch.cc:130: if (exception_) {
> Is this so you can allow a user of this class to just eat the exception?
>
> If so, I think the documentation in the header should say they can pass
> a NULL var for that.
>
> OTOH, I actually think it would be better to just do it unconditionally,
> and force the user to pass a valid var. That way, they at least have to
> think about it and be explicit if they're ignoring it. (Just seeing NULL
> at the call site is easier to overlook than an unused out-param)
>
>
https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
> File content/renderer/pepper/pepper_try_catch.h (right):
>
>
https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
> content/renderer/pepper/pepper_try_catch.h:58: PP_Var exception_;
> It looks like using scoped_refptr<StringVar> might be a little cleaner
> and protect against ref-count mistakes?
>
>
https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
> content/renderer/pepper/pepper_try_catch.h:69: PP_Var* exception);
> ScopedPPVar*, maybe?
>
> Otherwise the user of PepperTryCatchVar has to be careful to take
> ownership of the string's ref-count when an exception is set.
>
>
https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
> content/renderer/pepper/pepper_try_catch.h:78: // We typically don't
> have a HandleScope when using PepperTryCatchVar.
> Maybe a clearer wording would be:
> // Code which uses PepperTryCatchVar doesn't typically have a
> HandleScope, so we make one
> // for them. Note both HandleScope and PepperTryCatch are always stack
> variables.
> ...or something.
>
> (Alternately you could just make clients make the HandleScope
> themselves. This feels slightly weird to me, but I can see why you did
> it.)
>
>
https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
> File content/renderer/pepper/v8object_var.cc (right):
>
>
https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
> content/renderer/pepper/v8object_var.cc:1: // Copyright (c) 2011 The
> Chromium Authors. All rights reserved.
> nit: "(c) 2011"->"2014" (header too)
>
> https://codereview.chromium.org/421963008/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[raymes, 2014-08-06 03:22:52]

Thanks for taking a look!

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
File content/renderer/pepper/pepper_plugin_instance_impl.cc (right):

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_plugin_instance_impl.cc:668:
container_->element().document().frame()->mainWorldScriptContext();
On 2014/07/30 19:34:13, dmichael wrote:
> The frame can be NULL in rare cases, so please check that and return a null
> Context in that case.

Done.

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
File content/renderer/pepper/pepper_try_catch.cc (right):

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.cc:122: if (!GetContext().IsEmpty())
On 2014/07/30 19:34:14, dmichael wrote:
> This should never be empty, right? Maybe you can just:
>   GetContext()->Exit();
> unconditionally?

Done.

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.cc:130: if (exception_) {
There are two cases when we use this class:
-One is where the plugin code calls in and provides an out-param exception. In
this case, it handles the lifetime.
-We use this class internally in which case we don't care about the out-param
exception and we always pass NULL. 

We could have two constructors but it probably doesn't make much difference.
I've updated the comment for now, but let me know

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
File content/renderer/pepper/pepper_try_catch.h (right):

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.h:58: PP_Var exception_;
On 2014/07/30 19:34:14, dmichael wrote:
> It looks like using scoped_refptr<StringVar> might be a little cleaner and
> protect against ref-count mistakes?

Done.

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.h:69: PP_Var* exception);
This class is mainly used to provide an exception to the plugin. The plugin
takes ownership of the exception.

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.h:78: // We typically don't have a
HandleScope when using PepperTryCatchVar.
Done, I reworded the comment! I think it makes the code which uses this clearer
keeping the HandleScope here.

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
File content/renderer/pepper/v8object_var.cc (right):

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/v8object_var.cc:1: // Copyright (c) 2011 The Chromium
Authors. All rights reserved.
On 2014/07/30 19:34:14, dmichael wrote:
> nit: "(c) 2011"->"2014" (header too)

Done.

[raymes, 2014-08-06 03:29:56]

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
File content/renderer/pepper/pepper_try_catch.h (right):

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.h:58: PP_Var exception_;
Actually this won't work. We want to be able to pass a pointer to the PP_Var to
the plugin in calls that we make to it, so that it can write an exception there
(see exception()). This is why I didn't used a scoped thingy. It makes this
class a little more complex, but it simplifies all the uses.

[dmichael (off chromium), 2014-08-06 15:41:45]

lgtm

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
File content/renderer/pepper/pepper_try_catch.cc (right):

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.cc:122: if (!GetContext().IsEmpty())
On 2014/08/06 03:22:52, raymes wrote:
> On 2014/07/30 19:34:14, dmichael wrote:
> > This should never be empty, right? Maybe you can just:
> >   GetContext()->Exit();
> > unconditionally?
> 
> Done.
Hmm, I might have been wrong about this, sorry. Though I think in the case of an
empty Context the old code would have just broken someplace else.

It can wait until we see how the class gets used, but I think you might end up
wanting to make this class handle that case gracefully enough that we can
prevent crashing if a caller needs to use a PepperTryCatchVar but the plugin has
no context. We see this case for surf-away or tab closures sometimes, and it's
been a common source of crashers for us.

I think being blindly optimistic in the latest patchset is good for this CL,
though :)

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.cc:130: if (exception_) {
On 2014/08/06 03:22:52, raymes wrote:
> There are two cases when we use this class:
> -One is where the plugin code calls in and provides an out-param exception. In
> this case, it handles the lifetime.
> -We use this class internally in which case we don't care about the out-param
> exception and we always pass NULL. 
> 
> We could have two constructors but it probably doesn't make much difference.
> I've updated the comment for now, but let me know

I was trying to say it might be clearer if you just require a PP_Var, and the
caller can choose whether to do anything with it. E.g., a caller who didn't care
about the exception would say:
PP_Var unused;
PepperTryCatchVar catch(instance, true, &unused);
...which just seems slightly more readable to me than...
PepperTryCatchVar catch(instance, true, NULL);

WDYT?

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
File content/renderer/pepper/pepper_try_catch.h (right):

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.h:58: PP_Var exception_;
On 2014/08/06 03:29:56, raymes wrote:
> Actually this won't work. We want to be able to pass a pointer to the PP_Var
to
> the plugin in calls that we make to it, so that it can write an exception
there
> (see exception()). This is why I didn't used a scoped thingy. It makes this
> class a little more complex, but it simplifies all the uses.
Ah, I see. Thanks for explaining.

[raymes, 2014-08-11 00:26:21]

The CQ bit was checked by raymes@chromium.org

[raymes, 2014-08-11 00:26:33]

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
File content/renderer/pepper/pepper_try_catch.cc (right):

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.cc:122: if (!GetContext().IsEmpty())
Good to know, let's leave the check out for now and see where we crash :)

Wondering how many times I'm gonna need to revert and reland the CL that flips
the switch :(

https://codereview.chromium.org/421963008/diff/70001/content/renderer/pepper/...
content/renderer/pepper/pepper_try_catch.cc:130: if (exception_) {
Ah I see. I think that this would make the intended behavior clearer. On the
other hand it seems verbose to force a caller to pass an unused param when we
know there are going to be callers like that.

My preference (for the sake of readability of the code which uses this) is to
allow NULL for now, or alternatively I can make a separate constructor which
takes no param. Let me know, I can change it in a followup CL.

[commit-bot: I haz the power, 2014-08-11 00:28:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/raymes@chromium.org/421963008/110001

[commit-bot: I haz the power, 2014-08-11 02:59:17]

Change committed as 288637