Adds some table manipulation methods that were missing.

Fixes 1083.  Adds

HTMLTableElement: createCaption, deleteCaption, insertRow, deleteRow;
HTMLTableRowElement: insertCell, deleteCell

[MikeSamuel, 2009-07-28 01:18:22]

http://codereview.appspot.com/98051/diff/1/2
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/98051/diff/1/2#newcode2586
Line 2586: return defaultTameNode(this.node___.insertCell(0 + index),
this.editable___);
What do you want (0 + index) to do?
It definitely does not guarantee that the result is an integer or that it is in
a sensible range.

http://codereview.appspot.com/98051/diff/1/2#newcode2627
Line 2627: if (!this.editable___) { throw new Error(NOT_EDITABLE); }
Why are create methods contingent on editability?

[metaweta, 2009-07-28 01:23:28]

On 2009/07/28 01:18:22, MikeSamuel wrote:
> http://codereview.appspot.com/98051/diff/1/2
> File src/com/google/caja/plugin/domita.js (right):
> 
> http://codereview.appspot.com/98051/diff/1/2#newcode2586
> Line 2586: return defaultTameNode(this.node___.insertCell(0 + index),
> this.editable___);
> What do you want (0 + index) to do?
> It definitely does not guarantee that the result is an integer or that it is
in
> a sensible range.

I just wanted it not to be an object; I don't think it matters, though.  What
would you prefer to have in that position?  Just "index", or something that
guarantees it's an integer?

> 
> http://codereview.appspot.com/98051/diff/1/2#newcode2627
> Line 2627: if (!this.editable___) { throw new Error(NOT_EDITABLE); }
> Why are create methods contingent on editability?

Don't know.  I just copied the existing ones.

[MikeSamuel, 2009-07-28 02:48:36]

http://codereview.appspot.com/98051/diff/1/2
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/98051/diff/1/2#newcode2586
Line 2586: return defaultTameNode(this.node___.insertCell(0 + index),
this.editable___);
Mike Stay said:
I just wanted it not to be an object; I don't think it matters, though.
What would you prefer to have in that position?  Just "index", or
something that guarantees it's an integer?

[MikeSamuel, 2009-07-28 02:53:04]

http://codereview.appspot.com/98051/diff/1/2
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/98051/diff/1/2#newcode2586
Line 2586: return defaultTameNode(this.node___.insertCell(0 + index),
this.editable___);
Not being an object is an improvement, but there's no reason to map the string
'foo__' to '0foo__'.

You should require that the input be valid because otherwise you're allowing
access to badly defined behavior.

You should require that the index be valid according to the spec:
http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-68927016

insertCell modified in DOM Level 2
    Insert an empty TD cell into this row. If index is -1
    or equal to the number of cells, the new cell is
    appended.
    Parameters

    index of type long
        The place to insert the cell, starting from 0.

It looks like valid values are integers in [-1, children.length].

[metaweta, 2009-07-28 16:42:59]

On 2009/07/28 02:53:04, MikeSamuel wrote:
> You should require that the input be valid because otherwise you're 
> allowing access to badly defined behavior.
> 
> You should require that the index be valid according to the spec:
> http://www.w3.org/TR/DOM-Level-2-HTML/html.html#ID-68927016

The spec says it should throw a DOMException INDEX_SIZE_ERR if it's not a long
in the range [-1, length].  So do I need to do the check and throw the
exception, or can I just pass index through and assume that the browser got it
right?

[DavidSarah, 2009-07-28 17:33:30]

On 2009/07/28 16:42:59, metaweta wrote:
> The spec says it should throw a DOMException INDEX_SIZE_ERR if it's
> not a long in the range [-1, length].  So do I need to do the check
> and throw the exception, or can I just pass index through and assume
> that the browser got it right?

Always check. The browser implementors tend to take even required errors as a
license to add poorly thought-out extensions (and in some cases the spec was
written after browsers were already doing something different).

[metaweta, 2009-07-28 17:53:13]

snapshotted

[metaweta, 2009-07-29 19:45:55]

On 2009/07/28 17:53:13, metaweta wrote:
> snapshotted

ping

[MikeSamuel, 2009-07-29 19:51:28]

http://codereview.appspot.com/98051/diff/9/10
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/98051/diff/9/10#newcode2590
Line 2590: index <= this.node___.cells.length) {
There's a bit of duplication of code now.

How about creating a guard like
function requireIntIn(idx, min, max) {
  if (idx !== (idx | 0) || idx < min || idx > max) {
    throw new Error(INDEX_SIZE_ERROR);
  }
}
?

[metaweta, 2009-07-29 20:33:15]

On 2009/07/29 19:51:28, MikeSamuel wrote:
> http://codereview.appspot.com/98051/diff/9/10
> File src/com/google/caja/plugin/domita.js (right):
> 
> http://codereview.appspot.com/98051/diff/9/10#newcode2590
> Line 2590: index <= this.node___.cells.length) {
> There's a bit of duplication of code now.
> 
> How about creating a guard like
> function requireIntIn(idx, min, max) {
>   if (idx !== (idx | 0) || idx < min || idx > max) {
>     throw new Error(INDEX_SIZE_ERROR);
>   }
> }
> ?

Good idea.  Done.

[MikeSamuel, 2009-07-29 21:00:52]

http://codereview.appspot.com/98051/diff/1007/14
File src/com/google/caja/plugin/domita.js (right):

http://codereview.appspot.com/98051/diff/1007/14#newcode2589
Line 2589: }
Where is TameTableRowElement used?
Does defaultTameNode need to use it for <TR>s?

[metaweta, 2009-07-29 22:59:41]

On 2009/07/29 21:00:52, MikeSamuel wrote:
> http://codereview.appspot.com/98051/diff/1007/14
> File src/com/google/caja/plugin/domita.js (right):
> 
> http://codereview.appspot.com/98051/diff/1007/14#newcode2589
> Line 2589: }
> Where is TameTableRowElement used?
> Does defaultTameNode need to use it for <TR>s?

Yes, though apparently I forgot to put it in.  I've added some tests to
domita_test_untrusted.

[MikeSamuel, 2009-07-29 23:32:38]

LGTM

http://codereview.appspot.com/98051/diff/17/1009
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/98051/diff/17/1009#newcode2461
Line 2461: table.rows.item(0).deleteCell(0);
Maybe assert that the count of cells in row 0 is 0.

[metaweta, 2009-07-30 00:40:40]

On 2009/07/29 23:32:38, MikeSamuel wrote:
> http://codereview.appspot.com/98051/diff/17/1009#newcode2461
> Line 2461: table.rows.item(0).deleteCell(0);
> Maybe assert that the count of cells in row 0 is 0.
Done

@3612