The default bug view has changed. See this FAQ.

Remove nsIDOMNSHTMLFrameElement

RESOLVED FIXED in mozilla10

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: Yazen Ghannam)

Tracking

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

Trunk
mozilla10
addon-compat, dev-doc-needed
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
It should be merged into nsIDOMHTMLFormElement
(Assignee)

Comment 1

6 years ago
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
(Assignee)

Comment 3

6 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
Status: NEW → ASSIGNED
(Assignee)

Comment 6

6 years ago
Created attachment 552577 [details] [diff] [review]
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?
(Assignee)

Updated

6 years ago
Attachment #552577 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #552577 - Flags: review+
(Reporter)

Comment 7

6 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
> 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!
(Assignee)

Comment 8

6 years ago
Created attachment 552628 [details] [diff] [review]
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?
(Reporter)

Comment 9

6 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.
(Assignee)

Comment 10

6 years ago
Created attachment 552827 [details] [diff] [review]
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.
(Reporter)

Comment 11

6 years ago
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.
(Assignee)

Comment 12

6 years ago
Created attachment 552851 [details] [diff] [review]
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.
Attachment #552577 - Attachment is obsolete: true
Attachment #552628 - Attachment is obsolete: true
Attachment #552827 - Attachment is obsolete: true
(Reporter)

Comment 13

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

Comment 14

6 years ago
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
(Reporter)

Comment 15

6 years ago
You'll need to update /content/html/content/test/test_bug389797.html, the rest seems unrelated.
(Assignee)

Comment 16

6 years ago
Created attachment 552899 [details] [diff] [review]
Updated test_bug389797.html

In test_bug389797.html, I replaced nsIDOMNSHTMLFrameElement with nsIDOMHTMLIFrameElement.
Attachment #552851 - Attachment is obsolete: true
(Assignee)

Comment 17

6 years ago
May I please have the patch pushed to a try server again? Thank you.
http://tbpl.mozilla.org/?tree=Try&rev=f053672e2e8f

Comment 19

6 years ago
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
(Reporter)

Comment 20

6 years ago
Looking good... You can find a reviewer at https://wiki.mozilla.org/Modules
Yazen, you should request review if the patch is done.
(Reporter)

Updated

6 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+
(Reporter)

Comment 23

6 years ago
Yazen, do you have the time to fix this nits, or would you prefer me to fix them for you?
(Assignee)

Comment 24

6 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.
(Reporter)

Comment 25

6 years ago
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
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
(Reporter)

Comment 27

6 years ago
What Ed said :)
Keywords: addon-compat, dev-doc-needed
Whiteboard: [good first bug][mentor=Ms2ger]
You need to log in before you can comment on or make changes to this bug.