Clean up Proxy Handler families

RESOLVED FIXED in mozilla34

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla34
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Currently, there's all sorts of plans to get a handler family. Everyone seems to have their own. We should come up with one clean way to do it. It will also help bug 1027425 as some poeple use function calls, which is silly.

Personally, I like DebugScopeProxy's approach: |int DebugScopeProxy::family = 0;|
(Assignee)

Comment 1

3 years ago
Created attachment 8477070 [details] [diff] [review]
Patch

This patch just standardizes things. The main thing that's useful here is that we remove ProxyFamily() from the DOM code, which helps in the constexpr effort in bug 1027425
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8477070 - Flags: review?(jorendorff)
Comment on attachment 8477070 [details] [diff] [review]
Patch

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

Sure, this is an improvement.

I guess the C++-ish way to do this kind of thing is to declare a type that's explicitly empty:
    struct FamilyTag {};
(Pretty sure the Standard guarantees that global/static objects of empty types all have distinct addresses.) But I don't care that much.

Anyway---while you're at it, why not add a brief comment on BaseProxyHandler::{mFamily,family} explaining what they're for?

::: dom/base/WindowNamedPropertiesHandler.h
@@ +15,5 @@
>  class WindowNamedPropertiesHandler : public BaseDOMProxyHandler
>  {
>  public:
>    WindowNamedPropertiesHandler()
> +    : BaseDOMProxyHandler(&family, /* hasPrototype = */ true)

Why isn't nullptr OK here? I'm not super familiar with the family stuff, but I thought we only provided a non-null family pointer for families of proxies that we needed to test for dynamically somewhere.
Attachment #8477070 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 3

3 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #2)
> Anyway---while you're at it, why not add a brief comment on
> BaseProxyHandler::{mFamily,family} explaining what they're for?
> 

Done. 
> ::: dom/base/WindowNamedPropertiesHandler.h
> @@ +15,5 @@
> >  class WindowNamedPropertiesHandler : public BaseDOMProxyHandler
> >  {
> >  public:
> >    WindowNamedPropertiesHandler()
> > +    : BaseDOMProxyHandler(&family, /* hasPrototype = */ true)
> 
> Why isn't nullptr OK here? I'm not super familiar with the family stuff, but
> I thought we only provided a non-null family pointer for families of proxies
> that we needed to test for dynamically somewhere.

It's fine. I got overzealous. Reverted.

https://hg.mozilla.org/integration/mozilla-inbound/rev/54d1a31bb34c
https://hg.mozilla.org/mozilla-central/rev/54d1a31bb34c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.