180709 - Update Mozilla to the DOM Level 2 HTML PR

Status: RESOLVED FIXED

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

Description

[Peter Van der Beken [:peterv]]

HTMLAppletElement
  attribute hspace: change type from DOMString to long
  attribute vspace: change type from DOMString to long

HTMLBaseFontElement
  attribute size: change type from DOMString to long

HTMLImageElement
  attribute border: change type from long to DOMString

HTMLObjectElement
  attribute hspace: change type from DOMString to long
  attribute vspace: change type from DOMString to long

HTMLTableElement
  insertRow: needs to handle -1 as appending
  deleteRow: needs to handle -1 as deleting last

HTMLTableRowElement
  insertCell: needs to handle -1 as appending
  deleteCell: needs to handle -1 as deleting last

HTMLTableSectionElement
  insertRow: needs to handle -1 as appending
  deleteRow: needs to handle -1 as deleting last

Comment 1

[Johnny Stenback (:jst)]

In addition to those changes we also need to change 'size' in HTMLInputElement
from "DOMString" to "unsigned long". (This is an old Mozilla bug, but this
change should be made at the same time when we make the other changes).

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 3

[Johnny Stenback (:jst)]

Where are the interface changes?

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: Interface changes

Oh, you need those too? ;-)

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 108553 [details] [diff] [review]
v1

>Index: html/content/src/nsHTMLBaseFontElement.cpp
>+  PRInt32 ec;
>+  PRInt32 intValue;
>+  intValue = stringValue.ToInteger(&ec);
>+  if (ec != 0) {
>+    return NS_ERROR_FAILURE;
>+  }

Don't you need to strip the '+' (if there is one) first? or can ToInteger
handle that?

>Index: html/content/src/nsHTMLImageElement.cpp
>-NS_IMPL_PIXEL_ATTR(nsHTMLImageElement, Border, border)
>+NS_IMPL_STRING_ATTR(nsHTMLImageElement, Border, border)

Does doing myImg.border = "5" work after doing this? i.e. does it give a 5
pixel border?

>Index: html/content/src/nsHTMLInputElement.cpp

Do you need code to change the unit when the type attribute is changed?

>Index: html/content/src/nsHTMLTableElement.cpp
>@@ -907,12 +902,25 @@
> NS_IMETHODIMP
> nsHTMLTableElement::DeleteRow(PRInt32 aValue)
> {
>-  nsCOMPtr<nsIDOMHTMLCollection> rows;
>+  if (aValue < -1) {
>+    return NS_ERROR_DOM_INDEX_SIZE_ERR;
>+  }
> 
>+  nsCOMPtr<nsIDOMHTMLCollection> rows;
>   GetRows(getter_AddRefs(rows));
>-  nsCOMPtr<nsIDOMNode> row;
> 
>-  nsresult rv = rows->Item(aValue, getter_AddRefs(row));
>+  nsresult rv;
>+  PRUint32 refIndex;
>+  if (aValue == -1) {
>+    rv = rows->GetLength(&refIndex);

You need to decrease refIndex by one.

>Index: html/content/src/nsHTMLTableRowElement.cpp
>@@ -314,45 +310,32 @@
...
>+  nsCOMPtr<nsIDOMNode> sectionNode;
>+  nsresult result = GetParentNode(getter_AddRefs(sectionNode));
>+  NS_ENSURE_SUCCESS(result, result);
>+
>+  return CallQueryInterface(sectionNode, aSection);

Feel free to s/result/rv/ since you're rewriting every use of the variable
anyway

>+    if (doInsert) {
>+      PRInt32 refIndex = PR_MAX(aIndex, 0);   

You don't need this. You've already thrown an index exception if aIndex < -1.
Or was throwing wrong?

>@@ -497,16 +475,25 @@
> NS_IMETHODIMP
> nsHTMLTableRowElement::DeleteCell(PRInt32 aValue)
> {
>-  if (aValue < 0)
>+  if (aValue < -1) {
>     return NS_ERROR_DOM_INDEX_SIZE_ERR;
>-  
>-  nsCOMPtr<nsIDOMHTMLCollection> cells;
>+  }
> 
>+  nsCOMPtr<nsIDOMHTMLCollection> cells;
>   GetCells(getter_AddRefs(cells));
> 
>-  nsCOMPtr<nsIDOMNode> cell;
>+  nsresult rv;
>+  PRUint32 refIndex;
>+  if (aValue == -1) {
>+    rv = cells->GetLength(&refIndex);

same here. decrease by one.

>Index: html/content/src/nsHTMLTableSectionElement.cpp
>@@ -203,44 +202,51 @@
> {
>   *aValue = nsnull;
> 
>+  if (aIndex < -1) {
>+    return NS_ERROR_DOM_INDEX_SIZE_ERR;
>+  }
>+
>   nsCOMPtr<nsIDOMHTMLCollection> rows;
>   GetRows(getter_AddRefs(rows));
> 
>   PRUint32 rowCount;
>   rows->GetLength(&rowCount);
> 
>-  PRBool doInsert = (aIndex < PRInt32(rowCount));
>+  if (aIndex > (PRInt32)rowCount) {
>+    return NS_ERROR_DOM_INDEX_SIZE_ERR;
>+  }
>+
>+  PRBool doInsert = ((aIndex < PRInt32(rowCount)) && (aIndex != -1));

Too many parethesis IMHO. loose at least the outermost one.

>+  nsCOMPtr<nsIHTMLContent> rowContent;
>   nsresult rv = NS_NewHTMLTableRowElement(getter_AddRefs(rowContent),
>                                           nodeInfo);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsIDOMNode> rowNode(do_QueryInterface(rowContent));
> 
>-  if (NS_SUCCEEDED(rv) && rowContent) {
>-    nsCOMPtr<nsIDOMNode> rowNode(do_QueryInterface(rowContent));
>+  if (rowNode) {

IMHO you only need to assert that rowNode implements nsIDOMNode. Same on a few
other places in the table elements i think, sorry, missed it before :(

>+    nsCOMPtr<nsIDOMNode> retChild;
>+
>+    if (doInsert) {
>+      PRInt32 refIndex = PR_MAX(aIndex, 0);   

shouldn't be needed here either

>+      rv = InsertBefore(rowNode, refRow, getter_AddRefs(retChild));
>+    } else {
>+      rv = AppendChild(rowNode, getter_AddRefs(retChild));
>     }
>+
>+    if (retChild) {
>+      CallQueryInterface(retChild, aValue);

wanna check that this QI succeeds?

>@@ -248,21 +254,33 @@
> NS_IMETHODIMP
> nsHTMLTableSectionElement::DeleteRow(PRInt32 aValue)
> {
>-  nsCOMPtr<nsIDOMHTMLCollection> rows;
>+  if (aValue < -1) {
>+    return NS_ERROR_DOM_INDEX_SIZE_ERR;
>+  }
> 
>+  nsCOMPtr<nsIDOMHTMLCollection> rows;
>   GetRows(getter_AddRefs(rows));
> 
>-  nsCOMPtr<nsIDOMNode> row;
>-
>-  rows->Item(aValue, getter_AddRefs(row));
>+  nsresult rv;
>+  PRUint32 refIndex;
>+  if (aValue == -1) {
>+    rv = rows->GetLength(&refIndex);

same same

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 108559 [details] [diff] [review]
Interface changes

r=sicking

Comment 7

[Peter Van der Beken [:peterv]]

> Don't you need to strip the '+' (if there is one) first? or can ToInteger
> handle that?

Last time I looked ToInteger handles that fine, yes.

> Does doing myImg.border = "5" work after doing this? i.e. does it give a 5
> pixel border?

Yes.

>>+  nsCOMPtr<nsIHTMLContent> rowContent;
>>  nsresult rv = NS_NewHTMLTableRowElement(getter_AddRefs(rowContent),
>>					    nodeInfo);
>>+  NS_ENSURE_SUCCESS(rv, rv);
>>+
>>+  nsCOMPtr<nsIDOMNode> rowNode(do_QueryInterface(rowContent));
>>
>>-  if (NS_SUCCEEDED(rv) && rowContent) {
>>-    nsCOMPtr<nsIDOMNode> rowNode(do_QueryInterface(rowContent));
>>+  if (rowNode) {
> 
> 
> IMHO you only need to assert that rowNode implements nsIDOMNode. Same on a few
> other places in the table elements i think, sorry, missed it before :(

Really? we've just created the object with NS_NewHTMLTableRowElement and are
sure it supports the interface.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 108559 [details] [diff] [review]
Interface changes

sr=jst

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> > IMHO you only need to assert that rowNode implements nsIDOMNode. Same on a few
> > other places in the table elements i think, sorry, missed it before :(

> Really? we've just created the object with NS_NewHTMLTableRowElement and are
> sure it supports the interface.

no assertion is fine too, but please remove the |if|

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: v2

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 108877 [details] [diff] [review]
v2

r=sicking

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 108877 [details] [diff] [review]
v2

- In nsHTMLTableRowElement::GetSection():

-  nsIDOMNode *sectionNode = nsnull;
-  nsresult result = GetParentNode(&sectionNode);
-  if (NS_SUCCEEDED(result) && (nsnull != sectionNode)) {
-    result =
sectionNode->QueryInterface(NS_GET_IID(nsIDOMHTMLTableSectionElement),
(void**)aSection);
-    NS_RELEASE(sectionNode);
-  }
-  return result;
+
+  nsCOMPtr<nsIDOMNode> sectionNode;
+  nsresult rv = GetParentNode(getter_AddRefs(sectionNode));
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  return CallQueryInterface(sectionNode, aSection);

You need to check that sectionNode is non-null here before calling QI on it, it
will be null if GetSection() is called on a table row w/o a parent.

- In nsHTMLTableRowElement::GetTable():

   nsresult result = GetParentNode(getter_AddRefs(sectionNode));
+  NS_ENSURE_SUCCESS(result, result);

-  if (NS_SUCCEEDED(result) && sectionNode) {
-    nsCOMPtr<nsIDOMNode> tableNode;
...
-  }
+  nsCOMPtr<nsIDOMNode> tableNode;
+  result = sectionNode->GetParentNode(getter_AddRefs(tableNode));

Same thing here, you don't know that sectionNode is non-null here. Check it.

Other than that, sr=jst

Comment 13

[Peter Van der Beken [:peterv]]

Checked in.