Last Comment Bug 666200 - support select.add(element, long before)
: support select.add(element, long before)
Status: RESOLVED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Makoto Kato [:m_kato]
:
: Andrew Overholt [:overholt]
Mentors:
http://www.whatwg.org/specs/web-apps/...
Depends on: 666199 675072
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-22 02:16 PDT by Makoto Kato [:m_kato]
Modified: 2011-07-29 08:11 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix v1 (9.50 KB, patch)
2011-06-27 01:46 PDT, Makoto Kato [:m_kato]
no flags Details | Diff | Splinter Review
fix v2 (9.36 KB, patch)
2011-07-12 18:24 PDT, Makoto Kato [:m_kato]
bugs: review+
Details | Diff | Splinter Review

Description Makoto Kato [:m_kato] 2011-06-22 02:16:12 PDT
We don't support select.add(element, long before) yet.
Comment 1 Makoto Kato [:m_kato] 2011-06-27 01:46:43 PDT
Created attachment 542110 [details] [diff] [review]
fix v1
Comment 2 Makoto Kato [:m_kato] 2011-07-12 18:24:00 PDT
Created attachment 545563 [details] [diff] [review]
fix v2
Comment 3 Olli Pettay [:smaug] 2011-07-13 02:53:33 PDT
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);
> };
Comment 4 Mounir Lamouri (:mounir) 2011-07-13 03:06:55 PDT
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?
Comment 5 Olli Pettay [:smaug] 2011-07-13 03:14:35 PDT
In case of Add, = nsnull is IMO ok, since that is what the spec has effectively.
Comment 6 Makoto Kato [:m_kato] 2011-07-21 03:26:41 PDT
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)

Note You need to log in before you can comment on or make changes to this bug.