Closed Bug 548828 Opened 14 years ago Closed 14 years ago

implement document.head

Categories

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

enhancement
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.3a5

People

(Reporter: paulirish, Assigned: may.gup)

References

()

Details

(4 keywords)

Attachments

(4 files, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_8; en-US) AppleWebKit/532.9 (KHTML, like Gecko) Chrome/4.0.304.0 Safari/532.9
Build Identifier: 

from:
http://www.whatwg.org/specs/web-apps/current-work/complete.html#the-html-element

document.head returns the first head element that is a child of the html element, if there is one, or null otherwise.

Reproducible: Always
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: html5
Version: unspecified → Trunk
Attached file Testcase
Mayank is a CS student in India, and is going to give this a shot.
Assignee: nobody → may.gup
Status: NEW → ASSIGNED
this is not a final patch , so please ignore the conformance to line spacing and line feed standards.

please comment..
Attachment #442711 - Flags: review?
Attachment #442711 - Flags: review? → review?(bzbarsky)
Comment on attachment 442711 [details]
document.head runs fine, please comment

The actual implementation of GetHead is ok.  Some other things that need to be fixed, though:

1)  No need for the "Bug - 548828" comments. That information will be available in the checkin comment.

2)  You don't need to add GetHeadContentExternal.  

3)  nsIDOMHTMLDocument is frozen.  You can't add a new property there.  Please add it to nsIDOMNSHTMLDocument instead, put it at the end of the interface, and change the IID.

4)  Not necessary but desirable: include a checkin comment and correctly set User: string in the patch so it's easier to check in.  If you are using mq, just setting the commit and user in the patch in the queue and then uploading it from .hg/patches or exporting and uploading the export should do the trick.

Thanks for doing this!
Attachment #442711 - Flags: review?(bzbarsky) → review-
Attached patch corrections made (obsolete) — Splinter Review
issues in the above comment fixed.
Attachment #442711 - Attachment is obsolete: true
Attachment #442774 - Flags: review?(bzbarsky)
Comment on attachment 442774 [details] [diff] [review]
corrections made

> +  NS_IMETHOD GetHead(nsIDOMHTMLElement * *aHead);

You shouldn't need that, since nsHTMLDocument has that NS_DECL_NSIDOMNSHTMLDOCUMENT bit further down.

Take that out, and looks good.

Might I suggest this as the checkin comment, though?  "Bug 538828.  Implement HTMLDocument.head.  r=bzbarsky"
Er, and even "548828" for the bug number... ;)
Attached patch corrections madeSplinter Review
correction made in the code and checkin comment as well
Attachment #442774 - Attachment is obsolete: true
Attachment #442879 - Flags: review?(bzbarsky)
Attachment #442774 - Flags: review?(bzbarsky)
Comment on attachment 442879 [details] [diff] [review]
corrections made

Looks good.  Please add a test too (separate changeset for that ok).
Attachment #442879 - Flags: review?(bzbarsky) → review+
The IDL isn't correct, the return type should be nsIDOMHTMLHeadElement rather than nsIDOMHTMLElement according to the specification. (Not a big deal, I suppose.)
Attached patch IDL fixed (obsolete) — Splinter Review
the patch is almost same as the one above , it also fixes the mentioned IDL.
Attachment #442934 - Flags: review?(bzbarsky)
Attachment #442934 - Attachment is obsolete: true
Attachment #442934 - Flags: review?(bzbarsky)
Attached patch IDL fixedSplinter Review
The patch is same as above, just that it also fixes the IDL.

Please comment.
Thanks a lot for taking the time and reviewing/commenting.
Attachment #442978 - Flags: review?(bzbarsky)
Comment on attachment 442978 [details] [diff] [review]
IDL fixed

r=bzbarsky

Still waiting on a changeset with tests for this before pushing.
Attachment #442978 - Flags: review?(bzbarsky) → review+
Mayank, ping?
Attached patch Mochitest - testcase (obsolete) — Splinter Review
test case with mochitest

please comment..
Attachment #444038 - Flags: review?(bzbarsky)
Comment on attachment 444038 [details] [diff] [review]
Mochitest - testcase

This looks fine, but isn't part of the official core3 test suite, right?  Please put it in bugs/ instead?
Attachment #444038 - Flags: review?(bzbarsky) → review-
Attached patch mochitest in bugs folder (obsolete) — Splinter Review
please commment..
Attachment #444291 - Flags: review?(bzbarsky)
Attachment #444038 - Attachment is obsolete: true
Mochitest - testcase
Attachment #444291 - Attachment is obsolete: true
Attachment #444444 - Flags: review?(bzbarsky)
Attachment #444291 - Flags: review?(bzbarsky)
Attachment #444444 - Flags: review?(bzbarsky) → review+
Comment on attachment 444444 [details] [diff] [review]
mochitest in bugs folder

Stealing review, r=jst
Comment on attachment 444444 [details] [diff] [review]
mochitest in bugs folder

I was actually reading this as you marked... please use |is(x,y,reason)| instead of |ok(x == y, reason)|.
Attachment #444444 - Flags: review+ → review-
I'll just fix up the s/ok/is/ thing before landing.
Comment on attachment 444444 [details] [diff] [review]
mochitest in bugs folder

r=me with the use of is().
Attachment #444444 - Flags: review- → review+
Pushed:
  http://hg.mozilla.org/mozilla-central/rev/a7c51c733bcc
  http://hg.mozilla.org/mozilla-central/rev/585079f6b2a3

Mayank, thanks for getting this fixed!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
sir, it was a bigger help to me...... i should  actually thank you .....

:) :) thank you sir :)
Depends on: 615659
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.3a5
Seems this wasn't documented
Keywords: dev-doc-needed
Finished documenting it, removing dev-doc-needed.
Keywords: dev-doc-needed
Marek: when we are finished documenting a feature, we transform the dev-doc-needed into dev-doc-complete. That way we know it has been documented :-)
Marek: when we are finished documenting a feature, we transform the dev-doc-needed into dev-doc-complete. That way we know it has been documented :-)

Btw, thanks for the documentation!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: