Closed Bug 666200 Opened 14 years ago Closed 14 years ago

support select.add(element, long before)

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: m_kato, Assigned: m_kato)

References

(Depends on 1 open bug, )

Details

(Keywords: dev-doc-complete, html5)

Attachments

(1 file, 1 obsolete file)

We don't support select.add(element, long before) yet.
Attached patch fix v1 (obsolete) — Splinter Review
Assignee: nobody → m_kato
Attached patch fix v2Splinter Review
Attachment #542110 - Attachment is obsolete: true
Attachment #545563 - Flags: review?(Olli.Pettay)
Comment on attachment 545563 [details] [diff] [review] fix v2 > NS_IMETHODIMP >+nsHTMLSelectElement::Add(nsIDOMHTMLElement* aElement, >+ nsIVariant* aBefore) >+{ >+ PRUint16 dataType; >+ nsresult rv = aBefore->GetDataType(&dataType); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // aBefore is omitted or null >+ if (dataType == nsIDataType::VTYPE_EMPTY) >+ return Add(aElement); if (expr) { stmt; } >+ >+ nsCOMPtr<nsISupports> supports; >+ nsCOMPtr<nsIDOMHTMLElement> beforeElement; >+ >+ // whether aBefore is nsIDOMHTMLElement... >+ rv = aBefore->GetAsISupports(getter_AddRefs(supports)); >+ if (NS_SUCCEEDED(rv)) >+ beforeElement = do_QueryInterface(supports); ditto >+ if (NS_SUCCEEDED(rv)) >+ beforeElement = do_QueryInterface(beforeNode); and here. > interface nsIDOMNSHTMLOptionCollection : nsISupports > { > attribute long selectedIndex; > > [noscript] void setOption(in long index, > in nsIDOMHTMLOptionElement option); > > [noscript] readonly attribute nsIDOMHTMLSelectElement select; > >- [optional_argc] void add(in nsIDOMHTMLOptionElement option, >- [optional] in long index); >+ void add(in nsIDOMHTMLOptionElement option, >+ [optional] in nsIVariant before); > void remove(in long index); > };
Attachment #545563 - Flags: review?(Olli.Pettay) → review+
Comment on attachment 545563 [details] [diff] [review] fix v2 Review of attachment 545563 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for this patch :) I had a quick looko at the patch. It looks pretty good minus a few nits. Though, I think re-writing the new nsHTMLSelectElement::Add to make it more readable will be a good thing. ::: content/html/content/src/nsHTMLSelectElement.cpp @@ +688,5 @@ > + NS_ENSURE_SUCCESS(rv, rv); > + > + // aBefore is omitted or null > + if (dataType == nsIDataType::VTYPE_EMPTY) > + return Add(aElement); The coding style require: if (foo) { bar; } @@ +696,5 @@ > + > + // whether aBefore is nsIDOMHTMLElement... > + rv = aBefore->GetAsISupports(getter_AddRefs(supports)); > + if (NS_SUCCEEDED(rv)) > + beforeElement = do_QueryInterface(supports); Why not: if (NS_SUCCEEDED(aBefore->GetAsISupports(getter_AddRefs(supports))) { beforeElement = do_QueryInterface(supports); NS_ENSURE_TRUE(beforeElement, NS_ERROR_DOM_SYNTAX_ERR); return Add(aElement, beforeElement); } And you don't have to check for !beforeElement after. @@ +702,5 @@ > + // otherwise, whether aBefore is long > + if (!beforeElement) { > + PRInt32 index; > + rv = aBefore->GetAsInt32(&index); > + if (NS_SUCCEEDED(rv)) { You could do something like: PRInt32 index; NS_ENSURE_SUCCESS(aBefore->GetAsInt32(&index), NS_ERROR_DOM_SYNTAX_ERR); @@ +705,5 @@ > + rv = aBefore->GetAsInt32(&index); > + if (NS_SUCCEEDED(rv)) { > + nsCOMPtr<nsIDOMNode> beforeNode; > + rv = Item(index, getter_AddRefs(beforeNode)); > + if (NS_SUCCEEDED(rv)) Why not: if (NS_SUCCEEDED(Item(index, getter_AddRefs(beforeNode)) { beforeElement = ...; } @@ +717,5 @@ > + > + // aBefore is invalid because it isn't element or long > + NS_ENSURE_TRUE(beforeElement, NS_ERROR_DOM_SYNTAX_ERR); > + > + return Add(aElement, beforeElement); Those last lines are not needed with the changes above, I think. ::: content/html/content/src/nsHTMLSelectElement.h @@ +614,5 @@ > > + /** > + * Insert aElement before the node given by aBefore > + */ > + nsresult Add(nsIDOMHTMLElement* aElement, nsIDOMHTMLElement* aBefore = nsnull); Is having aBefore default to null useful? I think we try to minimize arguments with a default value in the Mozilla code base. Olli should be able to confirm that. ::: dom/interfaces/html/nsIDOMHTMLSelectElement.idl @@ +69,5 @@ > attribute unsigned long length; > nsIDOMNode item(in unsigned long index); > nsIDOMNode namedItem(in DOMString name); > void add(in nsIDOMHTMLElement element, > + [optional] in nsIVariant before) Maybe you could add a comment to say which types are expected? ::: dom/interfaces/html/nsIDOMNSHTMLOptionCollectn.idl @@ +53,5 @@ > > [noscript] readonly attribute nsIDOMHTMLSelectElement select; > > + void add(in nsIDOMHTMLOptionElement option, > + [optional] in nsIVariant before); Maybe you could add a comment to say which types are expected?
Attachment #545563 - Flags: review+ → review?(Olli.Pettay)
In case of Add, = nsnull is IMO ok, since that is what the spec has effectively.
Attachment #545563 - Flags: review?(Olli.Pettay) → review+
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Depends on: 675072
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: