Makoto Kato [:m_kato] Assignee
	Description • 13 years ago

We don't support select.add(element, long before) yet.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 13 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);
> };

	Mounir Lamouri (:mounir)
	Comment 4 • 13 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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 13 years ago

In case of Add, = nsnull is IMO ok, since that is what the spec has effectively.

	Makoto Kato [:m_kato] Assignee
	Comment 6 • 13 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)

	(no longer active)
	Comment 7 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/dd0343fffa4b
http://hg.mozilla.org/mozilla-central/rev/4ac34951ca4a