909953 - Introduce a HTMLAllCollection class

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

I want to move more stuff into it, but that's for another bug.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 796293 [details] [diff] [review]
Patch v1

>From: Ms2ger <[e-mail]>
>
>diff --git a/content/base/public/nsINode.h b/content/base/public/nsINode.h
>--- a/content/base/public/nsINode.h
>+++ b/content/base/public/nsINode.h
>@@ -1743,16 +1743,28 @@ inline nsINode* NODE_FROM(C& aContent, D
> {
>   if (aContent)
>     return static_cast<nsINode*>(aContent);
>   return static_cast<nsINode*>(aDocument);
> }
> 
> NS_DEFINE_STATIC_IID_ACCESSOR(nsINode, NS_INODE_IID)
> 
>+inline nsISupports*
>+ToSupports(nsINode* aPointer)
>+{
>+  return aPointer;
>+}
>+
>+inline nsISupports*
>+ToCanonicalSupports(nsINode* aPointer)
>+{
>+  return aPointer;
>+}
Not sure how this is related, but ok change anyway.


>+HTMLAllCollection::GetObject(JSContext* aCx, JS::Handle<JSObject*> aObject,
>+                             ErrorResult& aRv)
>+{
>+  MOZ_ASSERT(aCx);
>+  MOZ_ASSERT(aObject);
>+
>+  if (!mObject) {
>+    JSAutoCompartment ac(aCx, aObject);
>+    mObject = JS_NewObject(aCx, &sHTMLDocumentAllClass, nullptr,
>+                           JS_GetGlobalForObject(aCx, aObject));
>+    if (!mObject) {
>+      aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
>+      return nullptr;
>+    }
>+
>+    // Make the JSObject hold a reference to the document.
>+    JS_SetPrivate(mObject, ToSupports(mDocument));
>+    NS_ADDREF(mDocument);
>+
>+    mDocument->PreserveWrapper(ToSupports(mDocument));
I sure hope we have wrapper here. We should have, as far as I see.


>+  }
>+
>+  return mObject;
Could you call xpc_UnmarkGrayObject here before returning.
I know it is missing from the current code.

> NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsHTMLDocument, nsDocument)
>-  tmp->mAll = nullptr;
>+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mAll)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mImages)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mApplets)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mEmbeds)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mLinks)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mAnchors)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mScripts)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mForms)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mFormControls)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mWyciwygChannel)
>   NS_IMPL_CYCLE_COLLECTION_UNLINK(mMidasCommandManager)
> NS_IMPL_CYCLE_COLLECTION_UNLINK_END
Couldn't you now use some macro to traverse/unlink all these?

Comment 2

[Andrew McCreight [:mccr8]]

I think the JS pointer in there makes it hard to use a macro.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Olli Pettay [:smaug] from comment #1)
> Comment on attachment 796293 [details] [diff] [review]
> Patch v1
> 
> >From: Ms2ger <[e-mail]>
> >
> >diff --git a/content/base/public/nsINode.h b/content/base/public/nsINode.h
> >--- a/content/base/public/nsINode.h
> >+++ b/content/base/public/nsINode.h
> >@@ -1743,16 +1743,28 @@ inline nsINode* NODE_FROM(C& aContent, D
> > {
> >   if (aContent)
> >     return static_cast<nsINode*>(aContent);
> >   return static_cast<nsINode*>(aDocument);
> > }
> > 
> > NS_DEFINE_STATIC_IID_ACCESSOR(nsINode, NS_INODE_IID)
> > 
> >+inline nsISupports*
> >+ToSupports(nsINode* aPointer)
> >+{
> >+  return aPointer;
> >+}
> >+
> >+inline nsISupports*
> >+ToCanonicalSupports(nsINode* aPointer)
> >+{
> >+  return aPointer;
> >+}
> Not sure how this is related, but ok change anyway.

I wanted to call ToSupports from HTMLAllCollection::GetObject, but I also didn't want to include nsDOMQS.h :)

> 
> >+HTMLAllCollection::GetObject(JSContext* aCx, JS::Handle<JSObject*> aObject,
> >+                             ErrorResult& aRv)
> >+{
> >+  MOZ_ASSERT(aCx);
> >+  MOZ_ASSERT(aObject);
> >+
> >+  if (!mObject) {
> >+    JSAutoCompartment ac(aCx, aObject);
> >+    mObject = JS_NewObject(aCx, &sHTMLDocumentAllClass, nullptr,
> >+                           JS_GetGlobalForObject(aCx, aObject));
> >+    if (!mObject) {
> >+      aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
> >+      return nullptr;
> >+    }
> >+
> >+    // Make the JSObject hold a reference to the document.
> >+    JS_SetPrivate(mObject, ToSupports(mDocument));
> >+    NS_ADDREF(mDocument);
> >+
> >+    mDocument->PreserveWrapper(ToSupports(mDocument));
> I sure hope we have wrapper here. We should have, as far as I see.

So assert mDocument->PreservingWrapper()?

> >+  }
> >+
> >+  return mObject;
> Could you call xpc_UnmarkGrayObject here before returning.
> I know it is missing from the current code.

Will do.

> > NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(nsHTMLDocument, nsDocument)
> >-  tmp->mAll = nullptr;
> >+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mAll)
> >   NS_IMPL_CYCLE_COLLECTION_UNLINK(mImages)
> >   NS_IMPL_CYCLE_COLLECTION_UNLINK(mApplets)
> >   NS_IMPL_CYCLE_COLLECTION_UNLINK(mEmbeds)
> >   NS_IMPL_CYCLE_COLLECTION_UNLINK(mLinks)
> >   NS_IMPL_CYCLE_COLLECTION_UNLINK(mAnchors)
> >   NS_IMPL_CYCLE_COLLECTION_UNLINK(mScripts)
> >   NS_IMPL_CYCLE_COLLECTION_UNLINK(mForms)
> >   NS_IMPL_CYCLE_COLLECTION_UNLINK(mFormControls)
> >   NS_IMPL_CYCLE_COLLECTION_UNLINK(mWyciwygChannel)
> >   NS_IMPL_CYCLE_COLLECTION_UNLINK(mMidasCommandManager)
> > NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> Couldn't you now use some macro to traverse/unlink all these?

That would lose

NS_ASSERTION(!nsCCUncollectableMarker::InGeneration(cb, tmp->GetMarkedCCGeneration()),
             "Shouldn't traverse nsHTMLDocument!");

Is that ok?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v2

We discussed this on IRC: http://logbot.glob.com.au/?c=mozilla%23content#c122511

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/4a89871f5c4b