Closed Bug 968766 Opened 6 years ago Closed 6 years ago

Cleanup HTMLAllCollection

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

Details

Attachments

(11 files)

22.60 KB, patch
jst
: review+
Details | Diff | Splinter Review
10.40 KB, patch
jst
: review+
Details | Diff | Splinter Review
3.83 KB, patch
jst
: review+
Details | Diff | Splinter Review
3.20 KB, patch
jst
: review+
Details | Diff | Splinter Review
2.61 KB, patch
jst
: review+
Details | Diff | Splinter Review
2.95 KB, patch
jst
: review+
Details | Diff | Splinter Review
2.42 KB, patch
jst
: review+
Details | Diff | Splinter Review
4.46 KB, patch
jst
: review+
Details | Diff | Splinter Review
9.01 KB, patch
jst
: review+
Details | Diff | Splinter Review
5.22 KB, patch
jst
: review+
Details | Diff | Splinter Review
12.35 KB, patch
jst
: review+
Details | Diff | Splinter Review
No description provided.
Blocks: 969030
Comment on attachment 8371397 [details] [diff] [review]
Part a: Move the custom HTMLAllCollection code from nsDOMClassInfo.cpp to HTMLAllCollection.cpp

r=jst, but it'd be really great if we could reasonably not duplicate the WrapNative() methods here. Maybe split them out into a shared header, or somesuch?
Attachment #8371397 - Flags: review?(jst) → review+
Attachment #8371398 - Flags: review?(jst) → review+
Attachment #8371399 - Flags: review?(jst) → review+
Attachment #8371400 - Flags: review?(jst) → review+
Attachment #8371401 - Flags: review?(jst) → review+
Attachment #8371402 - Flags: review?(jst) → review+
Comment on attachment 8371403 [details] [diff] [review]
Part g: Introduce an AllCollection helper function

+static HTMLAllCollection*
+AllCollection(JSObject* aObject)
+{
+  return GetDocument(aObject)->All();
+}

This is a trivial one-liner that seems to be used only in a single place. I'd say we just put this code inline where it's called instead of having a helper for it.

r=jst with that.
Attachment #8371403 - Flags: review?(jst) → review+
Attachment #8371404 - Flags: review?(jst) → review+
Comment on attachment 8371405 [details] [diff] [review]
Part i: Move the document.all NodeLists into a separate hashtable

- In xpcom/glue/nsCycleCollectionParticipant.h:

-#define NS_IMPL_CYCLE_COLLECTION_INHERITED_11(_class, _base, _f1, _f2, _f3, _f4, _f5, _f6, _f7, _f8, _f9, _f10, _f11) \
+#define NS_IMPL_CYCLE_COLLECTION_INHERITED_12(_class, _base, _f1, _f2, _f3, _f4, _f5, _f6, _f7, _f8, _f9, _f10, _f11, _f12) \

I'd say instead of replacing this macro temporarily here just create a new _12 version and leave it in the code. The next guy who needs it will appreciate it :)

r=jst
Attachment #8371405 - Flags: review?(jst) → review+
Attachment #8371406 - Flags: review?(jst) → review+
Comment on attachment 8371407 [details] [diff] [review]
Part k: Move the hashmap for named access to document.all into HTMLAllCollection

r=jst, but take previous comment about cycle collector macros into account here.
Attachment #8371407 - Flags: review?(jst) → review+
Thanks for the reviews!

(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #12)
> Comment on attachment 8371397 [details] [diff] [review]
> Part a: Move the custom HTMLAllCollection code from nsDOMClassInfo.cpp to
> HTMLAllCollection.cpp
> 
> r=jst, but it'd be really great if we could reasonably not duplicate the
> WrapNative() methods here. Maybe split them out into a shared header, or
> somesuch?

I agree that it's a bit fishy, but given that there's already a dozen not-quite-alike WrapNatives lying around the tree, and that I'm planning to remove them when I move document.all to WebIDL in the coming weeks, I'd rather keep the code duplicated for now, if that's okay with you.

(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #13)
> Comment on attachment 8371403 [details] [diff] [review]
> Part g: Introduce an AllCollection helper function
> 
> +static HTMLAllCollection*
> +AllCollection(JSObject* aObject)
> +{
> +  return GetDocument(aObject)->All();
> +}
> 
> This is a trivial one-liner that seems to be used only in a single place.
> I'd say we just put this code inline where it's called instead of having a
> helper for it.
> 
> r=jst with that.

I was planning to put the collection in the slot instead of the document, but it looks like that never happened. Will inline.

(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #14)
> Comment on attachment 8371405 [details] [diff] [review]
> Part i: Move the document.all NodeLists into a separate hashtable
> 
> - In xpcom/glue/nsCycleCollectionParticipant.h:
> 
> -#define NS_IMPL_CYCLE_COLLECTION_INHERITED_11(_class, _base, _f1, _f2, _f3,
> _f4, _f5, _f6, _f7, _f8, _f9, _f10, _f11) \
> +#define NS_IMPL_CYCLE_COLLECTION_INHERITED_12(_class, _base, _f1, _f2, _f3,
> _f4, _f5, _f6, _f7, _f8, _f9, _f10, _f11, _f12) \
> 
> I'd say instead of replacing this macro temporarily here just create a new
> _12 version and leave it in the code. The next guy who needs it will
> appreciate it :)
> 
> r=jst
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #15)
> Comment on attachment 8371407 [details] [diff] [review]
> Part k: Move the hashmap for named access to document.all into
> HTMLAllCollection
> 
> r=jst, but take previous comment about cycle collector macros into account
> here.

I don't know how likely that is, but will do :)
Flags: needinfo?(jst)
(In reply to :Ms2ger from comment #16)
> Thanks for the reviews!

You're most welcome, and thanks for the patches!

> (In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #12)
> > Comment on attachment 8371397 [details] [diff] [review]
> > Part a: Move the custom HTMLAllCollection code from nsDOMClassInfo.cpp to
> > HTMLAllCollection.cpp
> > 
> > r=jst, but it'd be really great if we could reasonably not duplicate the
> > WrapNative() methods here. Maybe split them out into a shared header, or
> > somesuch?
> 
> I agree that it's a bit fishy, but given that there's already a dozen
> not-quite-alike WrapNatives lying around the tree, and that I'm planning to
> remove them when I move document.all to WebIDL in the coming weeks, I'd
> rather keep the code duplicated for now, if that's okay with you.

Ah, in that case, I don't have a problem with landing this with the duplication.
Flags: needinfo?(jst)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.