Closed Bug 824603 Opened 7 years ago Closed 7 years ago

Convert DocumentType to WebIDL

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Details

Attachments

(2 files, 2 obsolete files)

No description provided.
Attachment #695601 - Flags: review?(bzbarsky)
Summary: Remove quickstubs for nsIDOMSVGElement → Convert DocumentType to WebIDL
Comment on attachment 695601 [details] [diff] [review]
Part 1: Enable binding for DocumentType

Shouldn't you be overriding WrapNode, not WrapObject?
Comment on attachment 695602 [details] [diff] [review]
Part 2: Move nsDOMDocumentType => mozilla::dom::DocumentType

Please don't use "#pragma once"; it apparently doesn't actually work right, especially if you include the file with different filenames as you do in this patch.

Also, why not wrap the .cpp in namespace decls too instead of "using"?
Attachment #695602 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 695602 [details] [diff] [review]
> 
> Also, why not wrap the .cpp in namespace decls too instead of "using"?

DOMCI_DATA and NS_NewDOMDocumentType aren't in namespace mozilla so it was simpler to just do "using".  Is there a benefit to wrapping it in namespace decls?
> DOMCI_DATA and NS_NewDOMDocumentType aren't in namespace mozilla

Right, so they'd need to be moved to the top of the file.

> Is there a benefit to wrapping it in namespace decls?

I just think it's somewhat cleaner...
Using WrapNode and with test fixes.
Attachment #695601 - Attachment is obsolete: true
Attachment #695601 - Flags: review?(bzbarsky)
Attachment #695665 - Flags: review?(bzbarsky)
Attachment #695602 - Attachment is obsolete: true
Attachment #695667 - Flags: review?(bzbarsky)
Comment on attachment 695665 [details] [diff] [review]
Part 1: Enable binding for DocumentType

Make WrapNode protected and mark it as MOZ_OVERRIDE?  r=me with that.
Attachment #695665 - Flags: review?(bzbarsky) → review+
Comment on attachment 695667 [details] [diff] [review]
Part 2: Move nsDOMDocumentType => mozilla::dom::DocumentType

>+#include "DocumentType.h"

mozilla/dom/DocumentType.h, please.

>+++ b/content/base/src/DocumentType.h

Please don't add the blank line at the end.

r=me
Attachment #695667 - Flags: review?(bzbarsky) → review+
Comment on attachment 695665 [details] [diff] [review]
Part 1: Enable binding for DocumentType

One more thing.  I think you need a hasInstanceInterface on this guy, because at least the firediff addon uses "instanceof DocumentType"...
https://hg.mozilla.org/mozilla-central/rev/0ed0898de7b5
https://hg.mozilla.org/mozilla-central/rev/d128b15214cb
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.