290350 - [FIXr]Plaintext editor serialization creates unnecessary selection object

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

I'm guessing that this code was originally written before nsIDocumentEncoder had
a SetRange method... in any case, we no longer need the selection object here.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 2

[Simon Fraser [no longer active]]

Comment on attachment 180735 [details] [diff] [review]
Proposed patch

Seems reasonable

Comment 3

[Kathleen :Brade]

Comment on attachment 180735 [details] [diff] [review]
Proposed patch

r=brade (yes, it is old code from before the apis changed)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180735 [details] [diff] [review]
Proposed patch

Requesting 1.8b2 approval for a bit more unneeded code removal.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 180735 [details] [diff] [review]
Proposed patch

a=brendan for 1.8b2.

/be

Comment 6

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 7

[[e-mail]]

Are these possible optimizations? Replacing the entire block with
docEncoder->SetNode(rootElement); would bypass the encoder's range code too.
Failing that, I would have thought the inner block could be replaced with
range->SelectNode(rootElement); or range->SelectNodeContents(rootElement);

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Replacing the entire block with docEncoder->SetNode(rootElement)

That would also serialize rootElement itself, which the current code doesn't do.

> I would have thought the inner block could be replaced with
> range->SelectNode(rootElement)

That has the same problem.

> or range->SelectNodeContents(rootElement);

That _should_ work, yes.  I'm not sure it's faster, and it definitely uses more
memory (SelectNodeContents uses GetChildNodes, instead of nsIContent apis), but
it'd make this code shorter, which is probably a good thing...

Simon, Jonas, Peter, thoughts?

Comment 9

[[e-mail]]

(In reply to comment #8)
>>Replacing the entire block with docEncoder->SetNode(rootElement)
>That would also serialize rootElement itself, which the current code doesn't do.
Ah, I can see that that might emit an extra newline or something.

>SelectNodeContents uses GetChildNodes, instead of nsIContent apis
Is there a bug filed on that?

Comment 10

[Peter Van der Beken [:peterv]]

I'm fine with either way. As you note the range code is pretty inefficient, so
both ways are not as efficient as they could be :-).

Comment 11

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

Yeah, I wouldn't worry too much about efficiency. I doubt it'll affect perf much
either way, and we should fix the range code to be better in any event.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Is there a bug filed on that?

It's not clear that it's a bug, since the range code needs to handle a wider
range of node types than the editor.

Jonas, Peter, was that a vote in favor of using SelectNodeContents here?

Comment 13

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

Doesn't matter much to me, but i think it seems like a good idea yes.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Use selectNodeContents

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 181184 [details] [diff] [review]
Use selectNodeContents

Requesting 1.8b2 approval for a tiny bit more code removal.

Comment 16

[Asa Dotzler [:asa]]

Comment on attachment 181184 [details] [diff] [review]
Use selectNodeContents

a=asa

Comment 17

[Boris Zbarsky [:bzbarsky]]

Checked that second patch in.