Closed Bug 677085 Opened 13 years ago Closed 13 years ago

Remove nsIDOMNSHTMLFrameElement

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: Ms2ger, Assigned: yghannam)

References

(Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(1 file, 4 obsolete files)

It should be merged into nsIDOMHTMLFormElement
Hasn't this already been resolved?

https://bugzilla.mozilla.org/show_bug.cgi?id=660433

If not, then I'd like to work on it.
nsIDOMNSHTMLFormElement is gone, but you could work on nsIDOMNSHTMLFrameElement
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
Status: NEW → ASSIGNED
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?
Attachment #552577 - Flags: review+
Attachment #552577 - Flags: review+
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
> NS_INTERFACE_TABLE_HEAD(nsGenericHTMLFrameElement)
>   NS_INTERFACE_TABLE_INHERITED2(nsGenericHTMLFrameElement,
>-                                nsIDOMNSHTMLFrameElement,
>+                                nsIDOMHTMLFrameElement,
>                                 nsIFrameLoaderOwner)

Then this would become

NS_INTERFACE_TABLE_INHERITED1(nsGenericHTMLFrameElement,
                              nsIFrameLoaderOwner)

Thanks for taking this on!
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?
The quickstubs part looks good, but you probably wouldn't want nsGenericHTMLFrameElement to inherit from nsIDOMHTMLFrameElement, and remove the NS_DECL_NSIDOMHTMLFRAMEELEMENT lines.
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,
> 
> NS_IMETHODIMP
> nsHTMLIFrameElement::GetContentDocument(nsIDOMDocument** aContentDocument)
> {
>   return nsGenericHTMLFrameElement::GetContentDocument(aContentDocument);
> }
> 
> NS_IMETHODIMP
>+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.
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:
    http://tbpl.mozilla.org/?tree=Try&rev=18ee7d788103
Results (out of 234 total builds):
    exception: 1
    success: 199
    warnings: 32
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/Ms2ger@gmail.com-18ee7d788103
You'll need to update /content/html/content/test/test_bug389797.html, the rest seems unrelated.
In test_bug389797.html, I replaced nsIDOMNSHTMLFrameElement with nsIDOMHTMLIFrameElement.
Attachment #552851 - Attachment is obsolete: true
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:
    http://tbpl.mozilla.org/?tree=Try&rev=f053672e2e8f
Results (out of 156 total builds):
    success: 135
    warnings: 18
    failure: 3
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/khuey@mozilla.com-f053672e2e8f
Looking good... You can find a reviewer at https://wiki.mozilla.org/Modules
Yazen, you should request review if the patch is done.
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?
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!
https://hg.mozilla.org/mozilla-central/rev/a287056e121e

Congratulations on your first patch in the tree! Hope to see you on IRC (http://irc.mozilla.org/) 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 :-)
Status: ASSIGNED → RESOLVED
Closed: 13 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.

Attachment

General

Creator:
Created:
Updated:
Size: