Extend V8ValueConverter::Strategy for the needs of Gin Java Bridge

Extend V8ValueConverter::Strategy for the needs of Gin Java Bridge

Gin Java Bridge requires some customization in JS values serialization:

 - 'undefined' values must be encoded, not skipped;

 - 'non-finite' doubles must be supported;

 - ArrayBuffers and ArrayBufferViews must be encoded differently.

BUG=355644
R=jochen@chromium.org

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

[jochen (gone - plz use gerrit), 2014-04-23 15:17:32]

lgtm

https://codereview.chromium.org/248903003/diff/1/content/public/renderer/v8_v...
File content/public/renderer/v8_value_converter.h (right):

https://codereview.chromium.org/248903003/diff/1/content/public/renderer/v8_v...
content/public/renderer/v8_value_converter.h:58: base::Value** out) const = 0;
what about providing a default implementation that just returns false, so you
don't have to touch all concrete implementations?

[mnaganov (inactive), 2014-04-23 15:23:12]

Thanks, Jochen! I agree with your comment, but since this can be done in more
than one way, let's discuss this.

https://codereview.chromium.org/248903003/diff/1/content/public/renderer/v8_v...
File content/public/renderer/v8_value_converter.h (right):

https://codereview.chromium.org/248903003/diff/1/content/public/renderer/v8_v...
content/public/renderer/v8_value_converter.h:58: base::Value** out) const = 0;
On 2014/04/23 15:17:33, jochen wrote:
> what about providing a default implementation that just returns false, so you
> don't have to touch all concrete implementations?

Are you suggesting making the Strategy class non-abstract with method
implementations that return false, or still having Strategy as an interface, but
adding an implementation class to be inherited from?

[mnaganov (inactive), 2014-04-24 14:49:25]

On 2014/04/23 15:23:12, Mikhail Naganov (Cr) wrote:
> Thanks, Jochen! I agree with your comment, but since this can be done in more
> than one way, let's discuss this.
> 
>
https://codereview.chromium.org/248903003/diff/1/content/public/renderer/v8_v...
> File content/public/renderer/v8_value_converter.h (right):
> 
>
https://codereview.chromium.org/248903003/diff/1/content/public/renderer/v8_v...
> content/public/renderer/v8_value_converter.h:58: base::Value** out) const = 0;
> On 2014/04/23 15:17:33, jochen wrote:
> > what about providing a default implementation that just returns false, so
you
> > don't have to touch all concrete implementations?
> 
> Are you suggesting making the Strategy class non-abstract with method
> implementations that return false, or still having Strategy as an interface,
but
> adding an implementation class to be inherited from?

Jochen, can you please comment on this?

[jochen (gone - plz use gerrit), 2014-04-24 14:59:37]

On 2014/04/24 14:49:25, Mikhail Naganov (Cr) wrote:
> On 2014/04/23 15:23:12, Mikhail Naganov (Cr) wrote:
> > Thanks, Jochen! I agree with your comment, but since this can be done in
more
> > than one way, let's discuss this.
> > 
> >
>
https://codereview.chromium.org/248903003/diff/1/content/public/renderer/v8_v...
> > File content/public/renderer/v8_value_converter.h (right):
> > 
> >
>
https://codereview.chromium.org/248903003/diff/1/content/public/renderer/v8_v...
> > content/public/renderer/v8_value_converter.h:58: base::Value** out) const =
0;
> > On 2014/04/23 15:17:33, jochen wrote:
> > > what about providing a default implementation that just returns false, so
> you
> > > don't have to touch all concrete implementations?
> > 
> > Are you suggesting making the Strategy class non-abstract with method
> > implementations that return false, or still having Strategy as an interface,
> but
> > adding an implementation class to be inherited from?
> 
> Jochen, can you please comment on this?

sorry, didn't see this

I'd make it non-abstract

[mnaganov (inactive), 2014-04-24 15:51:35]

On 2014/04/24 14:59:37, jochen wrote:
> On 2014/04/24 14:49:25, Mikhail Naganov (Cr) wrote:
> > On 2014/04/23 15:23:12, Mikhail Naganov (Cr) wrote:
> > > Thanks, Jochen! I agree with your comment, but since this can be done in
> more
> > > than one way, let's discuss this.
> > > 
> > >
> >
>
https://codereview.chromium.org/248903003/diff/1/content/public/renderer/v8_v...
> > > File content/public/renderer/v8_value_converter.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/248903003/diff/1/content/public/renderer/v8_v...
> > > content/public/renderer/v8_value_converter.h:58: base::Value** out) const
=
> 0;
> > > On 2014/04/23 15:17:33, jochen wrote:
> > > > what about providing a default implementation that just returns false,
so
> > you
> > > > don't have to touch all concrete implementations?
> > > 
> > > Are you suggesting making the Strategy class non-abstract with method
> > > implementations that return false, or still having Strategy as an
interface,
> > but
> > > adding an implementation class to be inherited from?
> > 
> > Jochen, can you please comment on this?
> 
> sorry, didn't see this
> 
> I'd make it non-abstract

Thanks! I did that and rolled back my changes to ActivityLogConverter.

[mnaganov (inactive), 2014-04-24 15:51:42]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-04-24 15:53:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/248903003/20001

[commit-bot: I haz the power, 2014-04-24 16:42:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-24 16:42:06]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg
  tryserver.chromium on linux_chromium_clang_dbg

[mnaganov (inactive), 2014-04-24 17:06:02]

The CQ bit was checked by mnaganov@chromium.org

[commit-bot: I haz the power, 2014-04-24 17:08:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/248903003/40001

[commit-bot: I haz the power, 2014-04-24 21:07:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-24 21:07:19]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[mnaganov (inactive), 2014-04-25 09:07:11]

Committed patchset #3 manually as r266168 (presubmit successful).