Add toV8(value, scriptState)

Add toV8(value, scriptState)

BUG=https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/osvex5y2e24
R=haraken@chromium.org

Committed: https://crrev.com/28a97c58b94dfbaaee50375240d6b499d849245d
Cr-Commit-Position: refs/heads/master@{#370202}

[haraken, 2016-01-16 03:51:22]

LGTM

https://codereview.chromium.org/1591033002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ToV8.h (right):

https://codereview.chromium.org/1591033002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ToV8.h:247: // v8::Context::Global()
is not simply a getter.

What do you mean by "v8::Context::Global() is not simply a getter"?

[domenic, 2016-01-16 16:50:15]

On 2016/01/16 at 03:51:22, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/1591033002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/ToV8.h (right):
> 
>
https://codereview.chromium.org/1591033002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/ToV8.h:247: //
v8::Context::Global() is not simply a getter.
> 
> What do you mean by "v8::Context::Global() is not simply a getter"?

I meant that it seems to do nontrivial amounts of work and is not inline. I.e.
it's not just `inline ... v8::Context::Global() { return m_global; }`. Can you
think of a better way of phrasing it? Maybe "getter method"?

[haraken, 2016-01-17 02:56:53]

On 2016/01/16 16:50:15, domenic wrote:
> On 2016/01/16 at 03:51:22, haraken wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/1591033002/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/core/v8/ToV8.h (right):
> > 
> >
>
https://codereview.chromium.org/1591033002/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/core/v8/ToV8.h:247: //
> v8::Context::Global() is not simply a getter.
> > 
> > What do you mean by "v8::Context::Global() is not simply a getter"?
> 
> I meant that it seems to do nontrivial amounts of work and is not inline. I.e.
> it's not just `inline ... v8::Context::Global() { return m_global; }`. Can you
> think of a better way of phrasing it? Maybe "getter method"?

Makes sense. (I thought that v8::Context::Global has a side effect from your
original description.)

  "Use this function only if the call site does not otherwise need the global,
because v8::Context::Global is heavy."

?

[yhirano, 2016-01-17 23:51:37]

yhirano@chromium.org changed reviewers:
+ yhirano@chromium.org

[yhirano, 2016-01-17 23:51:38]

lgtm

https://codereview.chromium.org/1591033002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ToV8.h (right):

https://codereview.chromium.org/1591033002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ToV8.h:249: inline
v8::Local<v8::Value> toV8(T value, ScriptState* scriptState)
Is "T&&" better for avoiding copy?

[domenic, 2016-01-19 20:40:21]

On 2016/01/17 at 02:56:53, haraken wrote:

> Makes sense. (I thought that v8::Context::Global has a side effect from your
original description.)
> 
>   "Use this function only if the call site does not otherwise need the global,
because v8::Context::Global is heavy."
> 
> ?

Done, thanks.

On 2016/01/17 at 23:51:38, yhirano wrote:

> Is "T&&" better for avoiding copy?

Good point. After consulting StackOverflow
(http://stackoverflow.com/q/34885783/3191) I have determined I should use T&&
and std::forward to express my intent of just forwarding everything onward.

[domenic, 2016-01-19 20:41:04]

The CQ bit was checked by domenic@chromium.org

[domenic, 2016-01-19 20:41:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1591033002/#ps20001
(title: "Update in response to review comments")

[commit-bot: I haz the power, 2016-01-19 20:41:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1591033002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1591033002/20001

[commit-bot: I haz the power, 2016-01-19 22:14:23]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-19 22:16:54]

Description was changed from

==========
Add toV8(value, scriptState)

BUG=https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/osvex5y2e24
R=haraken@chromium.org
==========

to

==========
Add toV8(value, scriptState)

BUG=https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/osvex5y2e24
R=haraken@chromium.org

Committed: https://crrev.com/28a97c58b94dfbaaee50375240d6b499d849245d
Cr-Commit-Position: refs/heads/master@{#370202}
==========

[commit-bot: I haz the power, 2016-01-19 22:16:55]

Patchset 2 (id:??) landed as
https://crrev.com/28a97c58b94dfbaaee50375240d6b499d849245d
Cr-Commit-Position: refs/heads/master@{#370202}