Clean up Proxy Handler families

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla34
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Assignee

Description

5 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

5 years ago
Posted patch PatchSplinter Review
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

5 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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.