bindings: Move named/indexed attributes to prototype chains.

bindings: Move named/indexed attributes to prototype chains.

BUG=43394, 470671
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193448

[Yuki, 2015-04-08 09:44:01]

yukishiino@chromium.org changed reviewers:
+ haraken@chromium.org

[Yuki, 2015-04-08 09:44:02]

Could you review this CL?

[haraken, 2015-04-08 10:41:13]

I'm a bit confused, but don't we need to land a change like
https://codereview.chromium.org/1061243002/ before landing this change?

[Yuki, 2015-04-09 02:59:10]

On 2015/04/08 10:41:13, haraken wrote:
> I'm a bit confused, but don't we need to land a change like
> https://codereview.chromium.org/1061243002/ before landing this change?

Dan landed a CL: https://codereview.chromium.org/962613002/
which no longer masks existing properties, and after this change, he removed use
of HasRealNamedProperty and GetRealNamedPropertyInPrototypeChain at
https://codereview.chromium.org/1019773006 .

Before his changes:
1. namedProperty{Getter,Setter,etc.}Callback is always called.  It's the
callback function's duty to check if the property name is an existing property's
name or not.

After his changes:
1. Set kNonMasking flag in V8.
2. V8 checks if the property name is an existing property's name or not.
3. If it's an existing one, V8 calls a regular callback function for the
property.
4. If it's not an existing property, V8 calls
namedProperty{Getter,Setter,etc.}Callback.

We're already setting kNonMasking flag for auto-generated (i.e. non-custom)
binding code, however, not yet setting the flag for custom binding code.  Since
CSSStyleDeclaration has custom binding, we've not yet set kNonMasking and needed
to call HasRealNamedProperty and GetRealNamedPropertyInPrototypeChain.

Having said that, Dan recommended that we set kNonMasking for custom binding
code, too, and stop using HasRealNamedProperty and
GetRealNamedPropertyInPrototypeChain, which may return an empty v8::Local handle
without any exception.

I'm going to follow his advice and get rid of HasRealNamedProperty family.

[haraken, 2015-04-09 08:55:57]

On 2015/04/09 02:59:10, Yuki wrote:
> On 2015/04/08 10:41:13, haraken wrote:
> > I'm a bit confused, but don't we need to land a change like
> > https://codereview.chromium.org/1061243002/ before landing this change?
> 
> Dan landed a CL: https://codereview.chromium.org/962613002/
> which no longer masks existing properties, and after this change, he removed
use
> of HasRealNamedProperty and GetRealNamedPropertyInPrototypeChain at
> https://codereview.chromium.org/1019773006 .
> 
> Before his changes:
> 1. namedProperty{Getter,Setter,etc.}Callback is always called.  It's the
> callback function's duty to check if the property name is an existing
property's
> name or not.
> 
> After his changes:
> 1. Set kNonMasking flag in V8.
> 2. V8 checks if the property name is an existing property's name or not.
> 3. If it's an existing one, V8 calls a regular callback function for the
> property.
> 4. If it's not an existing property, V8 calls
> namedProperty{Getter,Setter,etc.}Callback.
> 
> We're already setting kNonMasking flag for auto-generated (i.e. non-custom)
> binding code, however, not yet setting the flag for custom binding code. 
Since
> CSSStyleDeclaration has custom binding, we've not yet set kNonMasking and
needed
> to call HasRealNamedProperty and GetRealNamedPropertyInPrototypeChain.
> 
> Having said that, Dan recommended that we set kNonMasking for custom binding
> code, too, and stop using HasRealNamedProperty and
> GetRealNamedPropertyInPrototypeChain, which may return an empty v8::Local
handle
> without any exception.
> 
> I'm going to follow his advice and get rid of HasRealNamedProperty family.

Thanks for the great details!

Also discussed offline, and I believe it's ready to land this CL. LGTM.

[Yuki, 2015-04-09 08:58:16]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2015-04-09 08:58:27]

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

[commit-bot: I haz the power, 2015-04-09 09:09:12]

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

[commit-bot: I haz the power, 2015-04-09 09:09:13]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[Yuki, 2015-04-09 09:40:59]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2015-04-09 09:41:00]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1062713003/#ps40001
(title: "Fixed layout tests.")

[commit-bot: I haz the power, 2015-04-09 09:41:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1062713003/40001

[commit-bot: I haz the power, 2015-04-09 14:57:54]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193448

[leviw_travelin_and_unemployed, 2015-04-09 20:34:53]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1078913002/ by leviw@chromium.org.

The reason for reverting is: Speculatively rolling out. Appears to be causing
massive Browser Test failures:
http://master.chrome.corp.google.com:8011/builders/cros%20trunk/builds/32444

crbug.com/475606.