Split lookup of WebIDL DOM class names from lookup of DOMCI DOM class names

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
5 months ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox48 affected, firefox49 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment)

Posted patch v1Splinter Review
This splits WebIDL name lookup out of nsDOMClassInfo. The size of the hash in nsScriptNameSpaceManager is currently 67184, after this patch it's 5744 (should hopefully be 0 eventually) and the new hash is 24624. Not super important since we have only 1 per process, but we want to do this anyway, so nice bonus.
Attachment #8744852 - Flags: review?(bzbarsky)
Whiteboard: btpp-active
Comment on attachment 8744852 [details] [diff] [review]
v1

So this changes behavior in case we ever end up with an IDL interface called "Components"... but I think that's fine.

Same for an interface named "controllers", but that's even less likely to be an issue.

>+++ b/dom/base/nsScriptNameSpaceManager.cpp
>+  WebIDLGlobalNameHash::Remove(aKey, key.Length());

The idea here is that if someone manually registers something with the script namespace manager that should override the existence of an IDL thing?  Is that just because we used to have that behavior?

+++ b/dom/bindings/BindingUtils.h
>+void
>+Register(nsScriptNameSpaceManager* aNameSpaceManager);

Why is this needed?  I don't think it is... the only caller was in RegisterDOMNames and went away.  So did the impl.

>+++ b/dom/bindings/Codegen.py
>+        str = fill("""

str() is a built-in Python thing/type, so it's probably better to use a different name here.

>+        str = ""

Similar here.

>+++ b/dom/bindings/WebIDLGlobalNameHash.cpp

>+static PLDHashTable* sWebIDLGlobalNames;

I assume there's a good reason nsTHashtable or one of its subclasses did not work here?  If so, please document.

But also, is the complexity of this worth it over just using a regular string key and doing copies as needed?  I mean, that's what our current code does, right?

>+WebIDLGlobalNameHash::Register(uint16_t aNameOffset, uint16_t aNameLength,
>+  const char* name = sNames + aNameOffset;

Can we MOZ_ASSERT(name[aNameLength] == '\0')?  If so, please do. If not, please tell me why in a comment!

>+WebIDLGlobalNameHash::Remove(const char* aName, uint32_t aLength)
>+  WebIDLNameTableKey webIDL(aName, aLength);

Call it "key" like in Register?

>+WebIDLGlobalNameHash::DefineIfEnabled(JSContext* aCx,
>+  *aFound = true;

So a found-but-not-exposed entry will prevent further lookups in the namespace manager?  I guess that's what we do right now as well... Seems a bit weird, but I can live with it, esp. if we're trying to nuke the namespace manager.

>+  JS::Rooted<JSObject*> global(aCx,

If this comes out non-null after the checked unwrap, can we assert that it's a window, or at least a global?

>+    FillPropertyDescriptor(aDesc, aObj, 0, JS::ObjectValue(*interfaceObject));

You need to include mozilla/dom/DOMJSProxyHandler.h in this file to get that without unified weirdness, right?

>+++ b/dom/bindings/WebIDLGlobalNameHash.h
>+  friend struct WebIDLNameTableEntry;

Please forward-declare this in the mozilla::dom namespace above.  Otherwise at least some compilers will treat that as a friend decl for mozilla::dom::WebIDLGlobalNameHash::WebIDLNameTableEntry iirc.

>+  // The value of sCount is generated by Codegen.py in RegisterBindings.cpp.

Yes, but please describe what it means.

>+  // The value of sNames is generated by Codegen.py in
>+  // RegisterBindings.cpp.

Please describe the structure of the generated array.

r=me.  This is awesome!
Attachment #8744852 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d3ea58c19507d754414b69bc7958d68ac80033
Bug 1267186 - Split lookup of WebIDL DOM class names from lookup of DOMCI DOM class names. r=bz.
> So this changes behavior in case we ever end up with an IDL interface called
> "Components"... but I think that's fine.
> 
> Same for an interface named "controllers", but that's even less likely to be an
> issue.

Yeah, we'll have an issue in that case even if I keep the current behaviour ;-).

> 
> > +++ b/dom/base/nsScriptNameSpaceManager.cpp
> > +  WebIDLGlobalNameHash::Remove(aKey, key.Length());
> 
> The idea here is that if someone manually registers something with the script
> namespace manager that should override the existence of an IDL thing?  Is that
> just because we used to have that behavior?

Right. We could change that, but keeping it is lower risk.

> > +static PLDHashTable* sWebIDLGlobalNames;
> 
> I assume there's a good reason nsTHashtable or one of its subclasses did not
> work here?  If so, please document.

I switched to nsTHashtable, it doesn't improve things much (does remove some casts though).

> But also, is the complexity of this worth it over just using a regular string
> key and doing copies as needed?  I mean, that's what our current code does,
> right?

We could keep the same setup as now, but I think the complexity is not huge and it does improve performance and memory usage.

> So a found-but-not-exposed entry will prevent further lookups in the namespace
> manager?  I guess that's what we do right now as well... Seems a bit weird, but
> I can live with it, esp. if we're trying to nuke the namespace manager.

If something is in the namespace manager we'd have removed the WebIDL one.
Backed out for static bustage in WebIDLGlobalNameHash.cpp.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b54e9f0fb23e
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=f9d3ea58c19507d754414b69bc7958d68ac80033
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=27382101&repo=mozilla-inbound
dom/bindings/WebIDLGlobalNameHash.cpp:57:12: error: Move constructors may not be marked explicit
Flags: needinfo?(peterv)
https://hg.mozilla.org/integration/mozilla-inbound/rev/d385df62c0e7f25dc846d4992638f1c63491a788
Bug 1267186 - Split lookup of WebIDL DOM class names from lookup of DOMCI DOM class names. r=bz.
https://hg.mozilla.org/integration/mozilla-inbound/rev/98e877355e47ec0e4382307d14f248e9b55472dc
Bug 1267186 - Split lookup of WebIDL DOM class names from lookup of DOMCI DOM class names. r=bz.
https://hg.mozilla.org/mozilla-central/rev/98e877355e47
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Flags: needinfo?(peterv)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.