support select.add(element, long before)

RESOLVED FIXED in mozilla8

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: m_kato, Assigned: m_kato)

Tracking

(Depends on: 1 bug, {dev-doc-complete, html5})

Trunk
mozilla8
dev-doc-complete, html5
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
We don't support select.add(element, long before) yet.
Keywords: dev-doc-needed
(Assignee)

Comment 1

6 years ago
Created attachment 542110 [details] [diff] [review]
fix v1
Assignee: nobody → m_kato
(Assignee)

Comment 2

6 years ago
Created attachment 545563 [details] [diff] [review]
fix v2
Attachment #542110 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #545563 - Flags: review?(Olli.Pettay)

Comment 3

6 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 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

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

Updated

6 years ago
Attachment #545563 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 6

6 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]
http://hg.mozilla.org/mozilla-central/rev/dd0343fffa4b
http://hg.mozilla.org/mozilla-central/rev/4ac34951ca4a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8

Updated

6 years ago
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 675072
You need to log in before you can comment on or make changes to this bug.