Closed
Bug 666200
Opened 14 years ago
Closed 14 years ago
support select.add(element, long before)
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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)
9.36 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
We don't support select.add(element, long before) yet.
Updated•14 years ago
|
Keywords: dev-doc-needed
Assignee | ||
Comment 1•14 years ago
|
||
Assignee: nobody → m_kato
Assignee | ||
Comment 2•14 years ago
|
||
Attachment #542110 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #545563 -
Flags: review?(Olli.Pettay)
Comment 3•14 years ago
|
||
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 4•14 years ago
|
||
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)
Comment 5•14 years ago
|
||
In case of Add, = nsnull is IMO ok, since that is what the spec has effectively.
Updated•14 years ago
|
Attachment #545563 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 6•14 years ago
|
||
land with smaug and volkmar's comments.
http://hg.mozilla.org/integration/mozilla-inbound/rev/dd0343fffa4b
http://hg.mozilla.org/integration/mozilla-inbound/rev/4ac34951ca4a (typo fix)
Whiteboard: [inbound]
Comment 7•14 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/dd0343fffa4b
http://hg.mozilla.org/mozilla-central/rev/4ac34951ca4a
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•