[api] Add documentation for PropertyCallbackInfo.

[api] Add documentation for PropertyCallbackInfo.

BUG=v8:5260
Committed: https://crrev.com/7a57dd1540d3ae5073f67dd5b9a5853d93c8e672
Cr-Commit-Position: refs/heads/master@{#38947}

[Franzi, 2016-08-23 08:37:49]

Description was changed from

==========
[api] Improve documentation for PropertyCallbackInfo.

BUG=v8:5260
==========

to

==========
[api] Add documentation for PropertyCallbackInfo.

BUG=v8:5260
==========

[Franzi, 2016-08-23 08:37:49]

franzih@chromium.org changed reviewers:
+ jochen@chromium.org

[Franzi, 2016-08-23 09:15:06]

Hi Jochen, 

I took a stab at adding documentation for the class PropertyCallbackInfo. Can
you have a look? 

Thanks, 

Franzi

[jochen (gone - plz use gerrit), 2016-08-23 12:43:26]

https://codereview.chromium.org/2263303003/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2263303003/diff/40001/include/v8.h#newcode3253
include/v8.h:3253: * intercepted.
Not sure I understand.

so assuming you have x and x's prototype is y, and y has an interceptor. then
This() would be x, but Holder() would be y.

https://codereview.chromium.org/2263303003/diff/40001/include/v8.h#newcode3260
include/v8.h:3260: * holder is `a`.
see above

[Franzi, 2016-08-23 13:11:01]

Hi Jochen, 

I completely misunderstood what Holder() is. Please take another look. Thanks, 

Franzi

https://codereview.chromium.org/2263303003/diff/40001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2263303003/diff/40001/include/v8.h#newcode3253
include/v8.h:3253: * intercepted.
On 2016/08/23 12:43:26, jochen wrote:
> Not sure I understand.
> 
> so assuming you have x and x's prototype is y, and y has an interceptor. then
> This() would be x, but Holder() would be y.

Done.

https://codereview.chromium.org/2263303003/diff/40001/include/v8.h#newcode3260
include/v8.h:3260: * holder is `a`.
On 2016/08/23 12:43:26, jochen wrote:
> see above

Done.

[jochen (gone - plz use gerrit), 2016-08-23 13:17:28]

lgtm, but please quickly verify with Toon that I didn't get This/Holder wrong
myself :D

[Franzi, 2016-08-23 13:19:03]

franzih@chromium.org changed reviewers:
+ verwaest@chromium.org

[Franzi, 2016-08-23 13:20:25]

On 2016/08/23 13:17:28, jochen wrote:
> lgtm, but please quickly verify with Toon that I didn't get This/Holder wrong
> myself :D

Hi Toon, 

Could you check if I got the documentation correct? 

Thanks, 

Franzi

[Toon Verwaest, 2016-08-23 13:28:34]

looking good

https://codereview.chromium.org/2263303003/diff/80001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2263303003/diff/80001/include/v8.h#newcode3253
include/v8.h:3253: * intercepted.
I guess the explanation is good enough, but we didn't necessarily go through
This() to get to this interceptor. We could have gone through Reflect.get, or
method.call(this, ..) where method through super['prop'] ended up calling the
interceptor with 'prop'.

The implications of this are that e.g., This could be an access-checked object,
but the access checks aren't necessarily triggered before we end up here. This
used to be a valid implicit assumption which would now lead to UXSS bugs.

https://codereview.chromium.org/2263303003/diff/80001/include/v8.h#newcode3260
include/v8.h:3260: * has an interceptor. Then `info.This()` is `x` and
`info.Holder()` is `y`.
Holder could be a hidden object (the global object, rather than the global
proxy).

This is important: the embedder is not allowed to pass that object back into the
runtime, otherwise this would again lead to UXSS.

https://codereview.chromium.org/2263303003/diff/80001/include/v8.h#newcode3274
include/v8.h:3274: * \return True, if the intercepted function should
drop the , after True

https://codereview.chromium.org/2263303003/diff/80001/include/v8.h#newcode3275
include/v8.h:3275: * throw if an error occurs. Usually, `true` corresponds to
`'use strict.'`
It's false if Reflect.get and friends are used; those return a boolean result
indicating whether or not the action failed.

[Franzi, 2016-08-24 09:04:05]

Hi Toon, 

Thanks for the comments. Please take another look. 

Thanks, 

Franzi

https://codereview.chromium.org/2263303003/diff/80001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/2263303003/diff/80001/include/v8.h#newcode3253
include/v8.h:3253: * intercepted.
On 2016/08/23 13:28:34, Toon Verwaest wrote:
> I guess the explanation is good enough, but we didn't necessarily go through
> This() to get to this interceptor. We could have gone through Reflect.get, or
> method.call(this, ..) where method through super['prop'] ended up calling the
> interceptor with 'prop'.
> 
> The implications of this are that e.g., This could be an access-checked
object,
> but the access checks aren't necessarily triggered before we end up here. This
> used to be a valid implicit assumption which would now lead to UXSS bugs.

IMHO poor documentation is worse than no documentation, so I deleted that
comment. I'll 
give it another try in a separate CL.

https://codereview.chromium.org/2263303003/diff/80001/include/v8.h#newcode3260
include/v8.h:3260: * has an interceptor. Then `info.This()` is `x` and
`info.Holder()` is `y`.
On 2016/08/23 13:28:34, Toon Verwaest wrote:
> Holder could be a hidden object (the global object, rather than the global
> proxy).
> 
> This is important: the embedder is not allowed to pass that object back into
the
> runtime, otherwise this would again lead to UXSS.

Added a comment about the hidden object and a warning about passing the object
back into the runtime.

https://codereview.chromium.org/2263303003/diff/80001/include/v8.h#newcode3274
include/v8.h:3274: * \return True, if the intercepted function should
On 2016/08/23 13:28:34, Toon Verwaest wrote:
> drop the , after True

Done.

https://codereview.chromium.org/2263303003/diff/80001/include/v8.h#newcode3275
include/v8.h:3275: * throw if an error occurs. Usually, `true` corresponds to
`'use strict.'`
On 2016/08/23 13:28:34, Toon Verwaest wrote:
> It's false if Reflect.get and friends are used; those return a boolean result
> indicating whether or not the action failed.

Done.

[Toon Verwaest, 2016-08-26 14:45:37]

lgtm

[Franzi, 2016-08-26 14:51:54]

The CQ bit was checked by franzih@chromium.org

[Franzi, 2016-08-26 14:51:55]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2263303003/#ps120001
(title: "Address review comments.")

[commit-bot: I haz the power, 2016-08-26 14:51:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 15:28:59]

Description was changed from

==========
[api] Add documentation for PropertyCallbackInfo.

BUG=v8:5260
==========

to

==========
[api] Add documentation for PropertyCallbackInfo.

BUG=v8:5260
==========

[commit-bot: I haz the power, 2016-08-26 15:29:00]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-26 15:29:33]

Description was changed from

==========
[api] Add documentation for PropertyCallbackInfo.

BUG=v8:5260
==========

to

==========
[api] Add documentation for PropertyCallbackInfo.

BUG=v8:5260

Committed: https://crrev.com/7a57dd1540d3ae5073f67dd5b9a5853d93c8e672
Cr-Commit-Position: refs/heads/master@{#38947}
==========

[commit-bot: I haz the power, 2016-08-26 15:29:34]

Patchset 7 (id:??) landed as
https://crrev.com/7a57dd1540d3ae5073f67dd5b9a5853d93c8e672
Cr-Commit-Position: refs/heads/master@{#38947}