Remove [OverrideBuiltins] from HTMLCollection

Remove [OverrideBuiltins] from HTMLCollection

This is basically a revert of r166628. While [OverrideBuiltins] helped with
Dromaeo's dom-query performance, it is against specification and it caused
the robohornet_pro benchmark to regress.

To minimize the performance impact of removing [OverrideBuiltins] on Dromaeo,
this CL generates slightly better code for the named property getter:
- Call HasRealNamedCallbackProperty() before GetRealNamedPropertyInPrototypeChain()
  as it is cheaper and more likely. It also makes sense to check the local object
  before traversing the prototype chain.
- Remove the HasRealNamedCallbackProperty() call as this is already included in
  HasRealNamedProperty(). Both call LocalLookupRealNamedProperty() internally then
  HasRealNamedCallbackProperty() does an extra IsPropertyCallbacks() check.

Note that Dromaeo's dom-query is still not as fast as with [OverrideBuiltins].
However, the performance hit is much lower now that the generated bindings code is
tweaked (-3.5% vs -15%).

R=haraken, arv
BUG=341192
BUG=341924
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166799

[arv (Not doing code reviews), 2014-02-08 18:13:58]

https://codereview.chromium.org/158273002/diff/1/Source/bindings/scripts/code...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/158273002/diff/1/Source/bindings/scripts/code...
Source/bindings/scripts/code_generator_v8.pm:4084: $code .= "    if
(info.Holder()->HasRealNamedProperty(name))\n";
I looked into this too.

I find this confusing. Maybe it is just bad V8 API names?

I would think we need to do something like:

1. Check own property for real property
2. Check prototype chain for real property
3. Check for own callback property
4. Check prototype chain for callback property

With this change, based on the (possibly bad) V8 names. It looks like we are
only doing 1 & 2.

Don't we need to look for callback properties to make sure that an ID/name is
not shadowing the length, item and namedItem?

https://codereview.chromium.org/158273002/diff/1/Source/bindings/scripts/code...
Source/bindings/scripts/code_generator_v8.pm:4086: $code .= "    if
(!info.Holder()->GetRealNamedPropertyInPrototypeChain(name).IsEmpty())\n";
So confusing. Why is this Get and not Has?

[Inactive, 2014-02-08 18:57:03]

https://codereview.chromium.org/158273002/diff/1/Source/bindings/scripts/code...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/158273002/diff/1/Source/bindings/scripts/code...
Source/bindings/scripts/code_generator_v8.pm:4084: $code .= "    if
(info.Holder()->HasRealNamedProperty(name))\n";
On 2014/02/08 18:13:58, arv wrote:
> I looked into this too.
> 
> I find this confusing. Maybe it is just bad V8 API names?
> 
> I would think we need to do something like:
> 
> 1. Check own property for real property
> 2. Check prototype chain for real property
> 3. Check for own callback property
> 4. Check prototype chain for callback property
> 
> With this change, based on the (possibly bad) V8 names. It looks like we are
> only doing 1 & 2.
> 
> Don't we need to look for callback properties to make sure that an ID/name is
> not shadowing the length, item and namedItem?

I looked at the V8 implementation and 
HasRealNamedProperty() works for callback properties as well. Therefore, I
believe the other check is redundant and can be removed.

https://codereview.chromium.org/158273002/diff/1/Source/bindings/scripts/code...
Source/bindings/scripts/code_generator_v8.pm:4086: $code .= "    if
(!info.Holder()->GetRealNamedPropertyInPrototypeChain(name).IsEmpty())\n";
On 2014/02/08 18:13:58, arv wrote:
> So confusing. Why is this Get and not Has?

This API does not have an Has equivalent for some reason, only a Get.

[Inactive, 2014-02-08 21:34:04]

https://codereview.chromium.org/158273002/diff/1/Source/bindings/scripts/code...
File Source/bindings/scripts/code_generator_v8.pm (right):

https://codereview.chromium.org/158273002/diff/1/Source/bindings/scripts/code...
Source/bindings/scripts/code_generator_v8.pm:4084: $code .= "    if
(info.Holder()->HasRealNamedProperty(name))\n";
On 2014/02/08 18:57:04, Chris Dumez wrote:
> On 2014/02/08 18:13:58, arv wrote:
> > I looked into this too.
> > 
> > I find this confusing. Maybe it is just bad V8 API names?
> > 
> > I would think we need to do something like:
> > 
> > 1. Check own property for real property
> > 2. Check prototype chain for real property
> > 3. Check for own callback property
> > 4. Check prototype chain for callback property
> > 
> > With this change, based on the (possibly bad) V8 names. It looks like we are
> > only doing 1 & 2.
> > 
> > Don't we need to look for callback properties to make sure that an ID/name
is
> > not shadowing the length, item and namedItem?
> 
> I looked at the V8 implementation and 
> HasRealNamedProperty() works for callback properties as well. Therefore, I
> believe the other check is redundant and can be removed.

Here is the implementation of those 2 methods if you want to compare:
http://pastebin.com/AvmgTwuV

[haraken, 2014-02-10 01:39:49]

LGTM, but I agree that the V8 API names are very confusing. I'd be happy if you
could rename them in v8.h and fix the binding side accordingly.

[Inactive, 2014-02-10 03:47:32]

The CQ bit was checked by ch.dumez@samsung.com

[commit-bot: I haz the power, 2014-02-10 03:47:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/158273002/1

[commit-bot: I haz the power, 2014-02-10 04:42:15]

Change committed as 166799