1192297 - Rephrase Proxy:: static methods to make it clearer that handler->hasPrototype() is the special case

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Here's the business end of Proxy::get():

    bool own;
    if (!handler->hasPrototype()) {
        own = true;
    } else {
        if (!handler->hasOwn(cx, proxy, id, &own))
            return false;
    }
    if (own)
        return handler->get(cx, proxy, receiver, id, vp);
    INVOKE_ON_PROTOTYPE(cx, handler, proxy, GetProperty(...));

I noticed this is a little hard to follow. It turns out the normal path is much clearer if you rewrite this code, without changing behavior, while holding in your mind a healthy dislike of mHasPrototype:

    if (handler->hasPrototype()) {
        bool own;
        if (!handler->hasOwn(cx, proxy, id, &own))
            return false;
        if (!own)
            INVOKE_ON_PROTOTYPE(cx, handler, proxy, GetProperty(...));
    }
    return handler->get(cx, proxy, receiver, id, vp);

So yeah, let's do this for all the methods tainted by mHasPrototype.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Rephrase Proxy static methods to make it clearer that the handler->hasPrototype() case is the weird case

Comment 2

[Jason Orendorff [:jorendorff]]

Attachment: Rephrase Proxy static methods to make it clearer that the handler->hasPrototype() case is the weird case

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 8645088 [details] [diff] [review]
Rephrase Proxy static methods to make it clearer that the handler->hasPrototype() case is the weird case

Review of attachment 8645088 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/proxy/Proxy.cpp
@@ +187,5 @@
>      MOZ_ASSERT(proxy->hasLazyPrototype());
>      JS_CHECK_RECURSION(cx, return false);
> +    if (!proxy->as<ProxyObject>().handler()->getPrototype(cx, proxy, proto))
> +        return false;
> +    assertSameCompartment(cx, proxy, proto);

This is fine, but I was thinking add this to js::GetPrototype.

@@ +245,5 @@
> +        if (!proto)
> +            return true;
> +
> +        bool found;
> +        if (!HasProperty(cx, proto, id, &found))

I'd just make this

  return HasProperty(cx, proto, id, bp);

and eliminate the |found| middleman.  Given the previous name for this was |Bp|, dollars to donuts says this only existed because of bool/JSBool mismatches.  I don't think it clarifies anything to have a separate variable here -- although, I would be in favor of renaming |bp| to |found| in all such hooks in a separate patch, if you really care about the name dumb.

Comment 4

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> > +    assertSameCompartment(cx, proxy, proto);
> 
> This is fine, but I was thinking add this to js::GetPrototype.

Mmmmmmmmmm, yeah, ok.

>   return HasProperty(cx, proto, id, bp);

Definitely.

Comment 5

[Jason Orendorff [:jorendorff]]

Adding that assertion to js::GetPrototype breaks causes it to fail a lot of the time, so I'll follow up in bug 888969, or a more specific followup if necessary. That we can't add that assertion is bad; it's probably necessary to fix it before bug 888969 can be fixed.

Comment 6

[Jason Orendorff [:jorendorff]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/b268c7a67b6256c66efa40fdd7b3a39480183738
changeset:  b268c7a67b6256c66efa40fdd7b3a39480183738
user:       Jason Orendorff <[e-mail]>
date:       Fri Aug 07 10:00:02 2015 -0500
description:
Bug 1192297 - Rephrase Proxy static methods to make it clearer that the handler->hasPrototype() case is the weird case. r=Waldo.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b268c7a67b62