Last Comment Bug 677085 - Remove nsIDOMNSHTMLFrameElement
: Remove nsIDOMNSHTMLFrameElement
Status: RESOLVED FIXED
: addon-compat, dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Yazen Ghannam
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: deCOM
  Show dependency treegraph
 
Reported: 2011-08-07 05:27 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2011-10-15 03:03 PDT (History)
6 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch to merge "nsIDOMNSHTMLFrameElement" to "nsIDOMHTMLFrameElement" and update all related files. (10.45 KB, patch)
2011-08-11 19:28 PDT, Yazen Ghannam
no flags Details | Diff | Splinter Review
Continuation of the patch before. (6.99 KB, patch)
2011-08-12 03:35 PDT, Yazen Ghannam
no flags Details | Diff | Splinter Review
More changes based on Ms2ger's advice. (2.61 KB, patch)
2011-08-12 20:35 PDT, Yazen Ghannam
no flags Details | Diff | Splinter Review
Final Patch. All patches have been merged into one. (16.36 KB, patch)
2011-08-13 02:25 PDT, Yazen Ghannam
no flags Details | Diff | Splinter Review
Updated test_bug389797.html (17.37 KB, patch)
2011-08-13 13:54 PDT, Yazen Ghannam
bzbarsky: review+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2011-08-07 05:27:25 PDT
It should be merged into nsIDOMHTMLFormElement
Comment 1 Yazen Ghannam 2011-08-11 15:48:46 PDT
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) 2011-08-11 15:53:34 PDT
nsIDOMNSHTMLFormElement is gone, but you could work on nsIDOMNSHTMLFrameElement
Comment 3 Yazen Ghannam 2011-08-11 15:55:35 PDT
Okay, thank you. I'll get started right away. Is it okay to assign this to myself?
Comment 4 David Zbarsky (:dzbarsky) 2011-08-11 15:59:47 PDT
Looks like nobody else is working on it, so go fot it.
Comment 5 Cameron McCormack (:heycam) 2011-08-11 16:10:23 PDT
(at Yazen's request)
Comment 6 Yazen Ghannam 2011-08-11 19:28:33 PDT
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?
Comment 7 :Ms2ger (⌚ UTC+1/+2) 2011-08-12 02:11:47 PDT
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 2011-08-12 03:35:52 PDT
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?
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2011-08-12 06:25:26 PDT
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 2011-08-12 20:35:19 PDT
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.
Comment 11 :Ms2ger (⌚ UTC+1/+2) 2011-08-13 01:15:38 PDT
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 2011-08-13 02:25:27 PDT
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.
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2011-08-13 03:42:41 PDT
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 2011-08-13 09:50:41 PDT
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
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2011-08-13 09:56:51 PDT
You'll need to update /content/html/content/test/test_bug389797.html, the rest seems unrelated.
Comment 16 Yazen Ghannam 2011-08-13 13:54:44 PDT
Created attachment 552899 [details] [diff] [review]
Updated test_bug389797.html

In test_bug389797.html, I replaced nsIDOMNSHTMLFrameElement with nsIDOMHTMLIFrameElement.
Comment 17 Yazen Ghannam 2011-08-15 03:35:10 PDT
May I please have the patch pushed to a try server again? Thank you.
Comment 18 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-08-15 03:48:53 PDT
http://tbpl.mozilla.org/?tree=Try&rev=f053672e2e8f
Comment 19 Mozilla RelEng Bot 2011-08-15 08:00:58 PDT
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
Comment 20 :Ms2ger (⌚ UTC+1/+2) 2011-08-15 08:06:18 PDT
Looking good... You can find a reviewer at https://wiki.mozilla.org/Modules
Comment 21 David Zbarsky (:dzbarsky) 2011-08-30 19:10:15 PDT
Yazen, you should request review if the patch is done.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2011-10-09 19:45:16 PDT
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 (⌚ UTC+1/+2) 2011-10-10 09:56:35 PDT
Yazen, do you have the time to fix this nits, or would you prefer me to fix them for you?
Comment 24 Yazen Ghannam 2011-10-10 21:11:52 PDT
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 (⌚ UTC+1/+2) 2011-10-10 23:27:05 PDT
OK, will do. Thanks for the patch!
Comment 26 Ed Morley [:emorley] 2011-10-15 02:53:05 PDT
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 (⌚ UTC+1/+2) 2011-10-15 03:03:05 PDT
What Ed said :)

Note You need to log in before you can comment on or make changes to this bug.