Closed
Bug 548828
Opened 14 years ago
Closed 14 years ago
implement document.head
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla1.9.3a5
People
(Reporter: paulirish, Assigned: may.gup)
References
()
Details
(4 keywords)
Attachments
(4 files, 5 obsolete files)
1.06 KB,
text/html
|
Details | |
1.93 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
2.57 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
2.27 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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
Comment 1•14 years ago
|
||
Here's a test page, linked to from the spec: http://ms2ger.freehostia.com/tests/html5/dom-tree-accessors/document.head-01.html
Updated•14 years ago
|
Updated•14 years ago
|
Keywords: student-project
Comment 2•14 years ago
|
||
Updated•14 years ago
|
Comment 3•14 years ago
|
||
Mayank is a CS student in India, and is going to give this a shot.
Assignee: nobody → may.gup
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•14 years ago
|
||
this is not a final patch , so please ignore the conformance to line spacing and line feed standards. please comment..
Attachment #442711 -
Flags: review?
Assignee | ||
Updated•14 years ago
|
Attachment #442711 -
Flags: review? → review?(bzbarsky)
Comment 5•14 years ago
|
||
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-
Assignee | ||
Comment 6•14 years ago
|
||
issues in the above comment fixed.
Attachment #442711 -
Attachment is obsolete: true
Attachment #442774 -
Flags: review?(bzbarsky)
Comment 7•14 years ago
|
||
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"
Comment 8•14 years ago
|
||
Er, and even "548828" for the bug number... ;)
Assignee | ||
Comment 9•14 years ago
|
||
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 10•14 years ago
|
||
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+
Comment 11•14 years ago
|
||
The IDL isn't correct, the return type should be nsIDOMHTMLHeadElement rather than nsIDOMHTMLElement according to the specification. (Not a big deal, I suppose.)
Assignee | ||
Comment 12•14 years ago
|
||
the patch is almost same as the one above , it also fixes the mentioned IDL.
Attachment #442934 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•14 years ago
|
Attachment #442934 -
Attachment is obsolete: true
Attachment #442934 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 13•14 years ago
|
||
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 14•14 years ago
|
||
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+
Comment 15•14 years ago
|
||
Mayank, ping?
Assignee | ||
Comment 16•14 years ago
|
||
test case with mochitest please comment..
Attachment #444038 -
Flags: review?(bzbarsky)
Comment 17•14 years ago
|
||
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-
Assignee | ||
Updated•14 years ago
|
Attachment #444038 -
Attachment is obsolete: true
Assignee | ||
Comment 19•14 years ago
|
||
Mochitest - testcase
Attachment #444291 -
Attachment is obsolete: true
Attachment #444444 -
Flags: review?(bzbarsky)
Attachment #444291 -
Flags: review?(bzbarsky)
Updated•14 years ago
|
Attachment #444444 -
Flags: review?(bzbarsky) → review+
Comment 20•14 years ago
|
||
Comment on attachment 444444 [details] [diff] [review] mochitest in bugs folder Stealing review, r=jst
Comment 21•14 years ago
|
||
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-
Comment 22•14 years ago
|
||
I'll just fix up the s/ok/is/ thing before landing.
Comment 23•14 years ago
|
||
Comment on attachment 444444 [details] [diff] [review] mochitest in bugs folder r=me with the use of is().
Attachment #444444 -
Flags: review- → review+
Comment 24•14 years ago
|
||
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
Assignee | ||
Comment 25•14 years ago
|
||
sir, it was a bigger help to me...... i should actually thank you ..... :) :) thank you sir :)
Updated•14 years ago
|
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.3a5
Comment 27•12 years ago
|
||
Started documenting this property here: https://developer.mozilla.org/en/DOM/document.head
Comment 29•12 years ago
|
||
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 :-)
Keywords: dev-doc-complete
Comment 30•12 years ago
|
||
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.
Description
•