Remove nsIDOMNSHTMLFrameElement

RESOLVED FIXED in mozilla10



8 years ago
8 years ago


(Reporter: Ms2ger, Assigned: yghannam)


(Blocks 1 bug, {addon-compat, dev-doc-needed})

Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(1 attachment, 4 obsolete attachments)

It should be merged into nsIDOMHTMLFormElement

Comment 1

8 years ago
Hasn't this already been resolved?

If not, then I'd like to work on it.
nsIDOMNSHTMLFormElement is gone, but you could work on nsIDOMNSHTMLFrameElement

Comment 3

8 years ago
Okay, thank you. I'll get started right away. Is it okay to assign this to myself?
Looks like nobody else is working on it, so go fot it.
Summary: Remove nsIDOMNSHTMLFormElement → Remove nsIDOMNSHTMLFrameElement
(at Yazen's request)
Assignee: nobody → yghannam

Comment 6

8 years ago
On line 138 of the patch, I removed the line containing "nsIDOMNSHTMLFrameElement" thinking that the line above it contained "nsIDOMHTMLFrameElement" and not "nsIDOMHTMLIFrameElement". Should I redo the patch so that the code at line 138 is replaced with "nsIDOMHTMLFrameElement" instead of deleting it?


8 years ago
Attachment #552577 - Flags: review+


8 years ago
Attachment #552577 - Flags: review+

Comment 7

8 years ago
Comment on attachment 552577 [details] [diff] [review]
Patch to merge "nsIDOMNSHTMLFrameElement" to "nsIDOMHTMLFrameElement" and update all related files.

So, this is a bit of a special case: nsIDOMNSHTMLFrameElement is used by both frame and iframe elements.

> On line 138 of the patch, I removed the line containing
> "nsIDOMNSHTMLFrameElement" thinking that the line above it contained
> "nsIDOMHTMLFrameElement" and not "nsIDOMHTMLIFrameElement". Should I redo
> the patch so that the code at line 138 is replaced with
> "nsIDOMHTMLFrameElement" instead of deleting it?

No. Instead you should add the contentWindow attribute to nsIDOMHTMLIFrameElement as well.

I think we should approach this somewhat differently, though. I would add a protected, non-virtual nsresult GetContentWindow(nsIDOMWindow** aContentWindow); to nsGenericHTMLFrameElement (next to the GetContentDocument it has), and add methods on nsHTMLFrameElement and nsHTMLIFrameElement, like we do for GetContentDocument.

>--- a/content/html/content/src/nsGenericHTMLElement.cpp
>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>-                                nsIDOMNSHTMLFrameElement,
>+                                nsIDOMHTMLFrameElement,
>                                 nsIFrameLoaderOwner)

Then this would become


Thanks for taking this on!

Comment 8

8 years ago
Thank you for your help Ms2ger. I've made a patch based on your suggestions. The only issue I've been having is that I get an error when I try to build using my patches. It always fails trying to build the "xpconnect libs". It mentions "dom_quickstubs.cpp" so I've tried removing the entries for "nsIDOMHTMLFrameElement.contentWindow" from "dom_quickstubs.qsconf" and adding "nsIDOMHTMLFrameElement.contentWindow" and every combination thereof. But it still fails at the same place. Any suggestions?

Comment 9

8 years ago
The quickstubs part looks good, but you probably wouldn't want nsGenericHTMLFrameElement to inherit from nsIDOMHTMLFrameElement, and remove the NS_DECL_NSIDOMHTMLFRAMEELEMENT lines.

Comment 10

8 years ago
Build fails with "found error" at <gkconhtmlcon_s.lib.desc>

This weekend I plan on spending more time studying the code and related topics. I'm already learning about DOM, XPCOM, and XPIDL. Are there any other topics that I should focus on also? Thanks again for all the help.
Comment on attachment 552628 [details] [diff] [review]
Continuation of the patch before.

>--- a/content/html/content/src/nsHTMLIFrameElement.cpp
>+++ b/content/html/content/src/nsHTMLIFrameElement.cpp
>@@ -134,16 +134,22 @@ NS_IMPL_STRING_ATTR(nsHTMLIFrameElement,
> nsHTMLIFrameElement::GetContentDocument(nsIDOMDocument** aContentDocument)
> {
>   return nsGenericHTMLFrameElement::GetContentDocument(aContentDocument);
> }
>+nsHTMLFrameElement::GetContentWindow(nsIDOMWindow** aContentWindow)

Does it work if you make this one nsHTML*I*FrameElement::GetContentWindow?

One warning if you're going to study our code: a lot of cruft has accumulated in the last decade or so. If you see unclear code, it's more likely that's the case because it was written unclearly, rather than because there's a well-hidden reason to do it that way.

Comment 12

8 years ago
Good catch! That indeed was a mistake on my part. But it wasn't the reason the build was failing. 

Apparently, "GetContentWindow" was defined (already) in nsGenericHTMLElement.cpp as returning "NS_IMETHODIMP" but it wasn't declared in nsGenericHTMLElement.h at all. So when I declared it in the header file, I put it with "GetContentDocument" as being protected and returning "nsresult". Of course, this was a mismatch and an error was thrown.

I guess the problem I had was reading the error messages. I kept looking at the final few errors instead of tracing up the 10-15 lines to find the first error. :P

In any case, the build was successful and I'm now running Nightly with this patch applied. Would someone be able to push the patch to a try server? I don't have access at this time. Thanks again for all the help.
Attachment #552577 - Attachment is obsolete: true
Attachment #552628 - Attachment is obsolete: true
Attachment #552827 - Attachment is obsolete: true
Ah, yes. I've sent the patch to try, a comment with the results should be added to this bug.
Try run for 18ee7d788103 is complete.
Detailed breakdown of the results available here:
Results (out of 234 total builds):
    exception: 1
    success: 199
    warnings: 32
    failure: 2
Builds available at
You'll need to update /content/html/content/test/test_bug389797.html, the rest seems unrelated.

Comment 16

8 years ago
In test_bug389797.html, I replaced nsIDOMNSHTMLFrameElement with nsIDOMHTMLIFrameElement.
Attachment #552851 - Attachment is obsolete: true

Comment 17

8 years ago
May I please have the patch pushed to a try server again? Thank you.
Try run for f053672e2e8f is complete.
Detailed breakdown of the results available here:
Results (out of 156 total builds):
    success: 135
    warnings: 18
    failure: 3
Builds available at
Looking good... You can find a reviewer at
Yazen, you should request review if the patch is done.


8 years ago
Attachment #552899 - Flags: review?(bzbarsky)
Comment on attachment 552899 [details] [diff] [review]
Updated test_bug389797.html

This looks pretty good!  Two nits:

1)  Fix the checkin comment; I'd probably just remove everything starting with the first "* * *".

2)  In test_bug389797.html you want to just remove the first entry from that array instead of changing it to "nsIDOMHTMLIFrameElement".  The test harness handles add "nsIDOMHTMLIFrameElement" to the interface name list already, right here:

49   var interfaceName = "nsIDOM" + getClassName(aTagName);
50   if (interfaceName in Components.interfaces) {  // no nsIDOMHTMLSpanElement
51     interfaces[aTagName].push(interfaceName);

r=me with those two nits fixed.
Attachment #552899 - Flags: review?(bzbarsky) → review+
Yazen, do you have the time to fix this nits, or would you prefer me to fix them for you?

Comment 24

8 years ago
Ms2ger, sorry I've been busy with work and school and I haven't had time to work on this. So yes, if you can go ahead and fix them for me I'd appreciate it. Thanks.
OK, will do. Thanks for the patch!

Congratulations on your first patch in the tree! Hope to see you on IRC ( in #developers soon. If you'd like to fix another bug (we'd love it if you did!) but need some inspiration, pop on & say hi - and we'll find something for you :-)
Closed: 8 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
What Ed said :)
Whiteboard: [good first bug][mentor=Ms2ger]
You need to log in before you can comment on or make changes to this bug.