IDL: Add support for [Unscopeable] on attributes and methods

Reland of IDL: Add support for [Unscopeable] on attributes and methods

Adding [Unscopeable] to an attribute or method means it won't be found
during name lookup with an object that has the attribute or method in
the scope (i.e. using the "with" statement.)

The mechanism that makes this happen is that the interface prototype
object has a property named @@unscopables that is an object with a
property for every unscopeable member named as the member.

BUG=462916
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194091

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194272

[Jens Widell, 2015-04-20 13:33:02]

jl@opera.com changed reviewers:
+ haraken@chromium.org

[Jens Widell, 2015-04-20 13:33:03]

PTAL

Note the slight weirdness that WebIDL has "unscopeable" (with an 'e') while
ECMAScript and thus V8 has "unscopable". WebIDL even misspells the symbol
@@unscopeables. Only the ECMAScript naming is in any way visible, so I guess
WebIDL will align, at least in how it spells the symbol.

[haraken, 2015-04-20 14:00:12]

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

[haraken, 2015-04-20 14:00:13]

LGTM

https://codereview.chromium.org/1085453003/diff/1/Source/bindings/templates/i...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/1085453003/diff/1/Source/bindings/templates/i...
Source/bindings/templates/interface_base.cpp:427:
prototypeTemplate->Set(v8::Symbol::GetUnscopables(isolate), unscopeables);

Should we check a return value of ForceSet and Set?

bashi@: What should we do when the ForceSet and Set fails? Just ignore, or
ToLocalChecked?

[Jens Widell, 2015-04-20 14:04:03]

On 2015/04/20 14:00:13, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/1085453003/diff/1/Source/bindings/templates/i...
> File Source/bindings/templates/interface_base.cpp (right):
> 
>
https://codereview.chromium.org/1085453003/diff/1/Source/bindings/templates/i...
> Source/bindings/templates/interface_base.cpp:427:
> prototypeTemplate->Set(v8::Symbol::GetUnscopables(isolate), unscopeables);
> 
> Should we check a return value of ForceSet and Set?

Template::Set() returns void.

> bashi@: What should we do when the ForceSet and Set fails? Just ignore, or
> ToLocalChecked?

[bashi, 2015-04-20 23:17:09]

On 2015/04/20 14:04:03, Jens Widell wrote:
> On 2015/04/20 14:00:13, haraken wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/1085453003/diff/1/Source/bindings/templates/i...
> > File Source/bindings/templates/interface_base.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1085453003/diff/1/Source/bindings/templates/i...
> > Source/bindings/templates/interface_base.cpp:427:
> > prototypeTemplate->Set(v8::Symbol::GetUnscopables(isolate), unscopeables);
> > 
> > Should we check a return value of ForceSet and Set?
> 
> Template::Set() returns void.
> 
> > bashi@: What should we do when the ForceSet and Set fails? Just ignore, or
> > ToLocalChecked?

Failures in installDOMTemplate seems fatal but I'm not sure what we want to do.
We may want to check failures using FromJust() then crash?

[Jens Widell, 2015-04-21 06:41:19]

On 2015/04/20 23:17:09, bashi1 wrote:
> On 2015/04/20 14:04:03, Jens Widell wrote:
> > On 2015/04/20 14:00:13, haraken wrote:
> > > LGTM
> > > 
> > >
> >
>
https://codereview.chromium.org/1085453003/diff/1/Source/bindings/templates/i...
> > > File Source/bindings/templates/interface_base.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1085453003/diff/1/Source/bindings/templates/i...
> > > Source/bindings/templates/interface_base.cpp:427:
> > > prototypeTemplate->Set(v8::Symbol::GetUnscopables(isolate), unscopeables);
> > > 
> > > Should we check a return value of ForceSet and Set?
> > 
> > Template::Set() returns void.
> > 
> > > bashi@: What should we do when the ForceSet and Set fails? Just ignore, or
> > > ToLocalChecked?
> 
> Failures in installDOMTemplate seems fatal but I'm not sure what we want to
do.
> We may want to check failures using FromJust() then crash?

Added FromJust() in PS#2.

[bashi, 2015-04-21 07:16:43]

LGTM

[Jens Widell, 2015-04-21 07:35:29]

The CQ bit was checked by jl@opera.com

[Jens Widell, 2015-04-21 07:35:29]

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/1085453003/#ps20001
(title: "call FromJust() on ForceSet()'s result")

[commit-bot: I haz the power, 2015-04-21 07:35:36]

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

[commit-bot: I haz the power, 2015-04-21 07:52:02]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194091

[alancutter (OOO until 2018), 2015-04-22 04:05:45]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1100053002/ by alancutter@chromium.org.

The reason for reverting is: This change caused 5k+ leaks on WebKit Linux Leak.
Confirmed via bisect.
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/bu....

[Jens Widell, 2015-04-22 10:19:02]

PS#3 moves the installation of @@unscopables from the prototype template to the
prototype object. For this purpose, I renamed
installConditionallyEnabledMethods() to preparePrototypeObject(), which deals
with both @@unscopables and conditionally enabled methods.

This rename of course lead to massive noise in the test results. The interesting
changes are in core/V8TestObject.{h,cpp} and modules/V8TestInterfacePartial.cpp.

I can do the rename in a separate CL if you prefer that (assuming you agree with
the rename approach at all, that is.)

[haraken, 2015-04-22 10:54:51]

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:930: {% from 'methods.cpp' import
install_conditionally_enabled_methods with context %}

install_conditionally_enabled_methods => prepare_prototype_object

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:937: {% if conditionally_enabled_methods
%}

Once we remove [PerContextEnabled], can we remove this branch completely?

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:950: {% if is_partial %}

Why do we need a different logic between partial and non-partial?

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:953: if (hasUnscopables)

if (!v8CallBoolean(...))

?

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
File Source/bindings/templates/partial_interface.cpp (right):

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/partial_interface.cpp:7: {% from 'methods.cpp' import
install_conditionally_enabled_methods

install_conditionally_enabled_methods => prepare_prototype_object

[Jens Widell, 2015-04-22 13:04:52]

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:930: {% from 'methods.cpp' import
install_conditionally_enabled_methods with context %}
On 2015/04/22 10:54:51, haraken wrote:
> 
> install_conditionally_enabled_methods => prepare_prototype_object

My thinking here is that the macro imported from methods.cpp really is (still)
dealing with specifically conditionally enabled methods. This block here
combines it with other things. Hence not changing the name.

But if you prefer renaming the macro (which isn't wrong; that is the
method-specific preparation of the prototype object) I'll happily change it.

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:937: {% if conditionally_enabled_methods
%}
On 2015/04/22 10:54:51, haraken wrote:
> 
> Once we remove [PerContextEnabled], can we remove this branch completely?

Yes, I imagine we could do that then.

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:950: {% if is_partial %}
On 2015/04/22 10:54:51, haraken wrote:
> 
> Why do we need a different logic between partial and non-partial?

We don't; the partial variant works for both, but is unnecessarily complex
(runtime) for the non-partial case, which can always assume there isn't already
a @@unscopables property/object on the prototype.

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:953: if (hasUnscopables)
On 2015/04/22 10:54:51, haraken wrote:
> 
> if (!v8CallBoolean(...))
> 
> ?

We can inline the HasOwnProperty() call here, yes. Negating would be wrong
though unless the true/false branches are swapped.

[haraken, 2015-04-22 13:10:32]

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:930: {% from 'methods.cpp' import
install_conditionally_enabled_methods with context %}
On 2015/04/22 13:04:51, Jens Widell wrote:
> On 2015/04/22 10:54:51, haraken wrote:
> > 
> > install_conditionally_enabled_methods => prepare_prototype_object
> 
> My thinking here is that the macro imported from methods.cpp really is (still)
> dealing with specifically conditionally enabled methods. This block here
> combines it with other things. Hence not changing the name.
> 
> But if you prefer renaming the macro (which isn't wrong; that is the
> method-specific preparation of the prototype object) I'll happily change it.

Makes sense, let's keep it. (Alternately I want to remove [PerContextEnabled]
and install_conditionally_enabled_methods.)

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:950: {% if is_partial %}
On 2015/04/22 13:04:52, Jens Widell wrote:
> On 2015/04/22 10:54:51, haraken wrote:
> > 
> > Why do we need a different logic between partial and non-partial?
> 
> We don't; the partial variant works for both, but is unnecessarily complex
> (runtime) for the non-partial case, which can always assume there isn't
already
> a @@unscopables property/object on the prototype.

Just to confirm: If we have X and PartialX, is it guaranteed that the
preparePrototypeObject of X is always called before the preparePrototypeObject
of PartialX?

If that is true, the preparePrototypeObject of PartialX will never hit the
branch for v8::Object::New(isolate).

[Jens Widell, 2015-04-22 14:22:10]

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:930: {% from 'methods.cpp' import
install_conditionally_enabled_methods with context %}
On 2015/04/22 13:10:32, haraken wrote:
> On 2015/04/22 13:04:51, Jens Widell wrote:
> > On 2015/04/22 10:54:51, haraken wrote:
> > > 
> > > install_conditionally_enabled_methods => prepare_prototype_object
> > 
> > My thinking here is that the macro imported from methods.cpp really is
(still)
> > dealing with specifically conditionally enabled methods. This block here
> > combines it with other things. Hence not changing the name.
> > 
> > But if you prefer renaming the macro (which isn't wrong; that is the
> > method-specific preparation of the prototype object) I'll happily change it.
> 
> Makes sense, let's keep it. (Alternately I want to remove [PerContextEnabled]
> and install_conditionally_enabled_methods.)

Yes, it would certainly be great to eliminate [PerContextEnabled]. But not in
this CL, I think. :-)

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:950: {% if is_partial %}
On 2015/04/22 13:10:32, haraken wrote:
> On 2015/04/22 13:04:52, Jens Widell wrote:
> > On 2015/04/22 10:54:51, haraken wrote:
> > > 
> > > Why do we need a different logic between partial and non-partial?
> > 
> > We don't; the partial variant works for both, but is unnecessarily complex
> > (runtime) for the non-partial case, which can always assume there isn't
> already
> > a @@unscopables property/object on the prototype.
> 
> Just to confirm: If we have X and PartialX, is it guaranteed that the
> preparePrototypeObject of X is always called before the preparePrototypeObject
> of PartialX?

Yes. V8FooPartial::preparePrototypeObject() calls
V8Foo::preparePrototypeObject() as its first action, before looking for
@@unscopable on the prototype object.

> If that is true, the preparePrototypeObject of PartialX will never hit the
> branch for v8::Object::New(isolate).

It will if only the partial interface has unscopeable members.

[haraken, 2015-04-22 15:36:10]

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:950: {% if is_partial %}
On 2015/04/22 14:22:10, Jens Widell wrote:
> On 2015/04/22 13:10:32, haraken wrote:
> > On 2015/04/22 13:04:52, Jens Widell wrote:
> > > On 2015/04/22 10:54:51, haraken wrote:
> > > > 
> > > > Why do we need a different logic between partial and non-partial?
> > > 
> > > We don't; the partial variant works for both, but is unnecessarily complex
> > > (runtime) for the non-partial case, which can always assume there isn't
> > already
> > > a @@unscopables property/object on the prototype.
> > 
> > Just to confirm: If we have X and PartialX, is it guaranteed that the
> > preparePrototypeObject of X is always called before the
preparePrototypeObject
> > of PartialX?
> 
> Yes. V8FooPartial::preparePrototypeObject() calls
> V8Foo::preparePrototypeObject() as its first action, before looking for
> @@unscopable on the prototype object.
> 
> > If that is true, the preparePrototypeObject of PartialX will never hit the
> > branch for v8::Object::New(isolate).
> 
> It will if only the partial interface has unscopeable members.

ok, understood.

I still prefer not introducing differnt logics between partial and non-partial
unless it really matters for performance. Can we do something like:

unscopeables = prototypeObject->Get(...);
if (!unscopeables.IsUndefined()) {
  unscopeables = v8::Object::New(isolate);
}

?

[Jens Widell, 2015-04-22 16:37:07]

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:950: {% if is_partial %}
On 2015/04/22 15:36:10, haraken wrote:
> On 2015/04/22 14:22:10, Jens Widell wrote:
> > On 2015/04/22 13:10:32, haraken wrote:
> > > On 2015/04/22 13:04:52, Jens Widell wrote:
> > > > On 2015/04/22 10:54:51, haraken wrote:
> > > > > 
> > > > > Why do we need a different logic between partial and non-partial?
> > > > 
> > > > We don't; the partial variant works for both, but is unnecessarily
complex
> > > > (runtime) for the non-partial case, which can always assume there isn't
> > > already
> > > > a @@unscopables property/object on the prototype.
> > > 
> > > Just to confirm: If we have X and PartialX, is it guaranteed that the
> > > preparePrototypeObject of X is always called before the
> preparePrototypeObject
> > > of PartialX?
> > 
> > Yes. V8FooPartial::preparePrototypeObject() calls
> > V8Foo::preparePrototypeObject() as its first action, before looking for
> > @@unscopable on the prototype object.
> > 
> > > If that is true, the preparePrototypeObject of PartialX will never hit the
> > > branch for v8::Object::New(isolate).
> > 
> > It will if only the partial interface has unscopeable members.
> 
> ok, understood.
> 
> I still prefer not introducing differnt logics between partial and non-partial
> unless it really matters for performance. Can we do something like:
> 
> unscopeables = prototypeObject->Get(...);
> if (!unscopeables.IsUndefined()) {
>   unscopeables = v8::Object::New(isolate);
> }
> 
> ?

We need to make sure to only read an own @@unscopables property from the
prototype object, hence my use of HasOwnProperty() before using Get() to
actually read the property.

But like I said above, we can use the current is_partial path for all cases;
HasOwnProperty() will just never return true for the !is_partial case.

[haraken, 2015-04-22 17:02:27]

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:950: {% if is_partial %}
On 2015/04/22 16:37:07, Jens Widell wrote:
> On 2015/04/22 15:36:10, haraken wrote:
> > On 2015/04/22 14:22:10, Jens Widell wrote:
> > > On 2015/04/22 13:10:32, haraken wrote:
> > > > On 2015/04/22 13:04:52, Jens Widell wrote:
> > > > > On 2015/04/22 10:54:51, haraken wrote:
> > > > > > 
> > > > > > Why do we need a different logic between partial and non-partial?
> > > > > 
> > > > > We don't; the partial variant works for both, but is unnecessarily
> complex
> > > > > (runtime) for the non-partial case, which can always assume there
isn't
> > > > already
> > > > > a @@unscopables property/object on the prototype.
> > > > 
> > > > Just to confirm: If we have X and PartialX, is it guaranteed that the
> > > > preparePrototypeObject of X is always called before the
> > preparePrototypeObject
> > > > of PartialX?
> > > 
> > > Yes. V8FooPartial::preparePrototypeObject() calls
> > > V8Foo::preparePrototypeObject() as its first action, before looking for
> > > @@unscopable on the prototype object.
> > > 
> > > > If that is true, the preparePrototypeObject of PartialX will never hit
the
> > > > branch for v8::Object::New(isolate).
> > > 
> > > It will if only the partial interface has unscopeable members.
> > 
> > ok, understood.
> > 
> > I still prefer not introducing differnt logics between partial and
non-partial
> > unless it really matters for performance. Can we do something like:
> > 
> > unscopeables = prototypeObject->Get(...);
> > if (!unscopeables.IsUndefined()) {
> >   unscopeables = v8::Object::New(isolate);
> > }
> > 
> > ?
> 
> We need to make sure to only read an own @@unscopables property from the
> prototype object, hence my use of HasOwnProperty() before using Get() to
> actually read the property.
> 
> But like I said above, we can use the current is_partial path for all cases;
> HasOwnProperty() will just never return true for the !is_partial case.

Makes sense. Then shall we use the current is_partial path for both cases?

[Jens Widell, 2015-04-22 17:08:04]

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:950: {% if is_partial %}
On 2015/04/22 17:02:26, haraken wrote:
> On 2015/04/22 16:37:07, Jens Widell wrote:
> > On 2015/04/22 15:36:10, haraken wrote:
> > > On 2015/04/22 14:22:10, Jens Widell wrote:
> > > > On 2015/04/22 13:10:32, haraken wrote:
> > > > > On 2015/04/22 13:04:52, Jens Widell wrote:
> > > > > > On 2015/04/22 10:54:51, haraken wrote:
> > > > > > > 
> > > > > > > Why do we need a different logic between partial and non-partial?
> > > > > > 
> > > > > > We don't; the partial variant works for both, but is unnecessarily
> > complex
> > > > > > (runtime) for the non-partial case, which can always assume there
> isn't
> > > > > already
> > > > > > a @@unscopables property/object on the prototype.
> > > > > 
> > > > > Just to confirm: If we have X and PartialX, is it guaranteed that the
> > > > > preparePrototypeObject of X is always called before the
> > > preparePrototypeObject
> > > > > of PartialX?
> > > > 
> > > > Yes. V8FooPartial::preparePrototypeObject() calls
> > > > V8Foo::preparePrototypeObject() as its first action, before looking for
> > > > @@unscopable on the prototype object.
> > > > 
> > > > > If that is true, the preparePrototypeObject of PartialX will never hit
> the
> > > > > branch for v8::Object::New(isolate).
> > > > 
> > > > It will if only the partial interface has unscopeable members.
> > > 
> > > ok, understood.
> > > 
> > > I still prefer not introducing differnt logics between partial and
> non-partial
> > > unless it really matters for performance. Can we do something like:
> > > 
> > > unscopeables = prototypeObject->Get(...);
> > > if (!unscopeables.IsUndefined()) {
> > >   unscopeables = v8::Object::New(isolate);
> > > }
> > > 
> > > ?
> > 
> > We need to make sure to only read an own @@unscopables property from the
> > prototype object, hence my use of HasOwnProperty() before using Get() to
> > actually read the property.
> > 
> > But like I said above, we can use the current is_partial path for all cases;
> > HasOwnProperty() will just never return true for the !is_partial case.
> 
> Makes sense. Then shall we use the current is_partial path for both cases?

Sure, I'll make that change.

[Jens Widell, 2015-04-22 17:15:13]

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:950: {% if is_partial %}
On 2015/04/22 17:08:04, Jens Widell wrote:
> On 2015/04/22 17:02:26, haraken wrote:
> > On 2015/04/22 16:37:07, Jens Widell wrote:
> > > On 2015/04/22 15:36:10, haraken wrote:
> > > > On 2015/04/22 14:22:10, Jens Widell wrote:
> > > > > On 2015/04/22 13:10:32, haraken wrote:
> > > > > > On 2015/04/22 13:04:52, Jens Widell wrote:
> > > > > > > On 2015/04/22 10:54:51, haraken wrote:
> > > > > > > > 
> > > > > > > > Why do we need a different logic between partial and
non-partial?
> > > > > > > 
> > > > > > > We don't; the partial variant works for both, but is unnecessarily
> > > complex
> > > > > > > (runtime) for the non-partial case, which can always assume there
> > isn't
> > > > > > already
> > > > > > > a @@unscopables property/object on the prototype.
> > > > > > 
> > > > > > Just to confirm: If we have X and PartialX, is it guaranteed that
the
> > > > > > preparePrototypeObject of X is always called before the
> > > > preparePrototypeObject
> > > > > > of PartialX?
> > > > > 
> > > > > Yes. V8FooPartial::preparePrototypeObject() calls
> > > > > V8Foo::preparePrototypeObject() as its first action, before looking
for
> > > > > @@unscopable on the prototype object.
> > > > > 
> > > > > > If that is true, the preparePrototypeObject of PartialX will never
hit
> > the
> > > > > > branch for v8::Object::New(isolate).
> > > > > 
> > > > > It will if only the partial interface has unscopeable members.
> > > > 
> > > > ok, understood.
> > > > 
> > > > I still prefer not introducing differnt logics between partial and
> > non-partial
> > > > unless it really matters for performance. Can we do something like:
> > > > 
> > > > unscopeables = prototypeObject->Get(...);
> > > > if (!unscopeables.IsUndefined()) {
> > > >   unscopeables = v8::Object::New(isolate);
> > > > }
> > > > 
> > > > ?
> > > 
> > > We need to make sure to only read an own @@unscopables property from the
> > > prototype object, hence my use of HasOwnProperty() before using Get() to
> > > actually read the property.
> > > 
> > > But like I said above, we can use the current is_partial path for all
cases;
> > > HasOwnProperty() will just never return true for the !is_partial case.
> > 
> > Makes sense. Then shall we use the current is_partial path for both cases?
> 
> Sure, I'll make that change.

Done.

https://codereview.chromium.org/1085453003/diff/40001/Source/bindings/templat...
Source/bindings/templates/interface.cpp:953: if (hasUnscopables)
On 2015/04/22 13:04:51, Jens Widell wrote:
> On 2015/04/22 10:54:51, haraken wrote:
> > 
> > if (!v8CallBoolean(...))
> > 
> > ?
> 
> We can inline the HasOwnProperty() call here, yes. Negating would be wrong
> though unless the true/false branches are swapped.

Done.

[haraken, 2015-04-22 17:21:15]

LGTM

[Jens Widell, 2015-04-23 05:33:09]

On 2015/04/22 17:21:15, haraken wrote:
> LGTM

Thanks.

I've confirmed that I can reproduce the leaks with the old patch, and that the
new version does not reproduce them. Relanding.

[Jens Widell, 2015-04-23 05:33:18]

The CQ bit was checked by jl@opera.com

[Jens Widell, 2015-04-23 05:33:19]

The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org
Link to the patchset: https://codereview.chromium.org/1085453003/#ps60001
(title: "fixes")

[commit-bot: I haz the power, 2015-04-23 05:33:31]

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

[commit-bot: I haz the power, 2015-04-23 08:04:59]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194272