Bug 6402 – Implement frameElement.width and frameElement.height

• Status: RESOLVED
• Resolution: FIXED
• Component: Frames

[Alexey Proskuryakov]

A partial fix for bug 4888. Implements two MSIE extensions for frameElement, width and height (which 
actually fixes the site).

See http://msdn.microsoft.com/workshop/author/dhtml/reference/objects/frame.asp

[Alexey Proskuryakov]

Created attachment 5518 [details]
proposed patch

The code here is rather different from other accessors around; perhaps I'm
doing something horribly wrong...

Also, not sure if alert() is safe to use in the test here.

[Eric Seidel (no email)]

Comment on attachment 5518 [details]
proposed patch

/me wonders frameWidth() and frameHeight() are virtual...

[Geoffrey Garen]

Comment on attachment 5518 [details]
proposed patch

I second Eric's question: why virtual?

The alert will work in the layout test, but it can be kinda annoying when you
open that page manually. An explanation of what you're testing and a reference
to the related bug are also important and missing. Check out
LayoutTests/fast/js/string-split-ignore-case.html for an example. It also
demonstrates how to produce output without an alert.

[Alexey Proskuryakov]

Created attachment 5521 [details]
revised patch

> /me wonders frameWidth() and frameHeight() are virtual...
My bad, copy/paste problem.

The usual way of printing the results to a "console" element doesn't work well
here - DRT doesn't dump frame contents in text mode, and changing the test to
pixel mode just because of this would only make checkouts longer. I've changed
the test to alert() in DRT, but document.write() in manual mode.

Maciej says: "in the past we've had problems with layout tests that made heavy
use of alert(). probably a single use would be ok. other option would be to not
dump as text. probably better to use the alert in this case."

[Darin Adler]

Comment on attachment 5521 [details]
revised patch

In the frameWidth and frameHeight functions, there's an assumption that both
getDocument() and m_render are non-0.

I'm certain that's not correct for m_render. You can easily get a pointer to a
frame element that no longer has a renderer (perhaps it was removed from the
DOM tree) and then calling the function will do a nil-deref.

Similarly, I believe there are circumstances where you could have a reference
to a frame element that does not have a document (although I'm not certain
about that one).

Please add the checks for 0.

Also seems better to not have a space in the header between frameWidth and
frameHeight declarations.

Looks fine otherwise.

[Alexey Proskuryakov]

Created attachment 5534 [details]
revised patch

Addressed Darin's comments (WinIE also returns 0 in this case).

[Maciej Stachowiak]

Elements now always have to have a document, so I think it's ok to just assert that document is non-0 
instead of checking for it. But fine as-is, r=me.