677085 - Remove nsIDOMNSHTMLFrameElement

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

It should be merged into nsIDOMHTMLFormElement

Comment 1

[Yazen Ghannam]

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.

Comment 2

[David Zbarsky (:dzbarsky)]

nsIDOMNSHTMLFormElement is gone, but you could work on nsIDOMNSHTMLFrameElement

Comment 3

[Yazen Ghannam]

Okay, thank you. I'll get started right away. Is it okay to assign this to myself?

Comment 4

[David Zbarsky (:dzbarsky)]

Looks like nobody else is working on it, so go fot it.

Comment 5

[Cameron McCormack (:heycam)]

(at Yazen's request)

Comment 6

[Yazen Ghannam]

Attachment: Patch to merge "nsIDOMNSHTMLFrameElement" to "nsIDOMHTMLFrameElement" and update all related files.

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?

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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!

Comment 8

[Yazen Ghannam]

Attachment: Continuation of the patch before.

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

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

[Yazen Ghannam]

Attachment: More changes based on Ms2ger's advice.

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 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 12

[Yazen Ghannam]

Attachment: Final Patch. All patches have been merged into one.

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.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Ah, yes. I've sent the patch to try, a comment with the results should be added to this bug.

Comment 14

[Mozilla RelEng Bot]

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/[e-mail]-18ee7d788103

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

You'll need to update /content/html/content/test/test_bug389797.html, the rest seems unrelated.

Comment 16

[Yazen Ghannam]

Attachment: Updated test_bug389797.html

In test_bug389797.html, I replaced nsIDOMNSHTMLFrameElement with nsIDOMHTMLIFrameElement.

Comment 17

[Yazen Ghannam]

May I please have the patch pushed to a try server again? Thank you.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://tbpl.mozilla.org/?tree=Try&rev=f053672e2e8f

Comment 19

[Mozilla RelEng Bot]

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/[e-mail]-f053672e2e8f

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Looking good... You can find a reviewer at https://wiki.mozilla.org/Modules

Comment 21

[David Zbarsky (:dzbarsky)]

Yazen, you should request review if the patch is done.

Comment 22

[Boris Zbarsky [: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.

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yazen, do you have the time to fix this nits, or would you prefer me to fix them for you?

Comment 24

[Yazen Ghannam]

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.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

OK, will do. Thanks for the patch!

Comment 26

[Ed Morley [:emorley]]

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 :-)

Comment 27

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

What Ed said :)