Bug 27478 – All DOMConstructorObjects should hold a pointer to the JSDOMGlobalObject

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Eric Seidel (no email)]

All DOMConstructorObjects should hold a pointer to the JSDOMGlobalObject

All DOMConstructorObject subclasses are already passed a JSDOMGlobalObject* during construction.  Only 4 of them actually hold onto this pointer though.

We should make all constructor objects hold onto this pointer, and share the marking and casting code using a subclass instead of the current copy/paste code.

[Eric Seidel (no email)]

Created attachment 33135 [details]
patch

[Adam Barth]

+ explicit DOMObjectWithGlobalPointer(PassRefPtr<JSC::Structure> structure, JSDOMGlobalObject* globalObject)

Explicit only makes sense for one-argument constructors.

WebCore/bindings/js/JSMessageChannelConstructor.cpp
+     , m_globalObject(globalObject)

Can't we remove this line now?

Maciej had some suggestions in the channel.  I'll wait for you to address those comments before reviewing this in more detail.

[Eric Seidel (no email)]

Created attachment 33140 [details]
Update patch

[Adam Barth]

Comment on attachment 33140 [details]
Update patch

This patch is awesome.  Redundant code: BE GONE!

+        I've added two new classes: DOMObjectWithGlobalPointer and DOMConstructorObjectInDocument.

The ChangeLog uses the old class names.

+    class DOMConstructorWithDocument : public DOMConstructorObject {
+    public:
+        Document* document() const
+        {
+            return static_cast<Document*>(scriptExecutionContext());
+        }
+    protected:

Missing a blank line here.

[Eric Seidel (no email)]

(In reply to comment #4)
> (From update of attachment 33140 [details])
> This patch is awesome.  Redundant code: BE GONE!
> 
> +        I've added two new classes: DOMObjectWithGlobalPointer and
> DOMConstructorObjectInDocument.
> 
> The ChangeLog uses the old class names.

My bad.  Will fix.

> +    class DOMConstructorWithDocument : public DOMConstructorObject {
> +    public:
> +        Document* document() const
> +        {
> +            return static_cast<Document*>(scriptExecutionContext());
> +        }
> +    protected:
> 
> Missing a blank line here.

Where?  Before protected?

[Adam Barth]

> Where?  Before protected?

Yes.

[Eric Seidel (no email)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/JSAudioConstructor.cpp
	M	WebCore/bindings/js/JSAudioConstructor.h
	M	WebCore/bindings/js/JSDOMBinding.h
	M	WebCore/bindings/js/JSImageConstructor.cpp
	M	WebCore/bindings/js/JSImageConstructor.h
	M	WebCore/bindings/js/JSMessageChannelConstructor.cpp
	M	WebCore/bindings/js/JSMessageChannelConstructor.h
	M	WebCore/bindings/js/JSOptionConstructor.cpp
	M	WebCore/bindings/js/JSOptionConstructor.h
	M	WebCore/bindings/js/JSWebKitCSSMatrixConstructor.cpp
	M	WebCore/bindings/js/JSWebKitPointConstructor.cpp
	M	WebCore/bindings/js/JSWorkerConstructor.cpp
	M	WebCore/bindings/js/JSXMLHttpRequestConstructor.cpp
	M	WebCore/bindings/js/JSXMLHttpRequestConstructor.h
	M	WebCore/bindings/js/JSXSLTProcessorConstructor.cpp
Committed r46191
	M	WebCore/ChangeLog
	M	WebCore/bindings/js/JSXMLHttpRequestConstructor.h
	M	WebCore/bindings/js/JSAudioConstructor.h
	M	WebCore/bindings/js/JSWorkerConstructor.cpp
	M	WebCore/bindings/js/JSOptionConstructor.h
	M	WebCore/bindings/js/JSWebKitPointConstructor.cpp
	M	WebCore/bindings/js/JSOptionConstructor.cpp
	M	WebCore/bindings/js/JSXMLHttpRequestConstructor.cpp
	M	WebCore/bindings/js/JSImageConstructor.cpp
	M	WebCore/bindings/js/JSDOMBinding.h
	M	WebCore/bindings/js/JSMessageChannelConstructor.h
	M	WebCore/bindings/js/JSAudioConstructor.cpp
	M	WebCore/bindings/js/JSWebKitCSSMatrixConstructor.cpp
	M	WebCore/bindings/js/JSImageConstructor.h
	M	WebCore/bindings/js/JSXSLTProcessorConstructor.cpp
	M	WebCore/bindings/js/JSMessageChannelConstructor.cpp
r46191 = df9f155678da8d894d73bfb7bf58ab24e14fc872 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46191