[webkit-reviews] review requested: [Bug 52268] Setting outerText should convert CR/LF to <br> : [Attachment 78735] Patch

[bugzilla-daemon at webkit.org]

Emil A Eklund <eae at chromium.org> has asked  for review:
Bug 52268: Setting outerText should convert CR/LF to <br>
https://bugs.webkit.org/show_bug.cgi?id=52268

Attachment 78735: Patch
https://bugs.webkit.org/attachment.cgi?id=78735&action=review

------- Additional Comments from Emil A Eklund <eae at chromium.org>
Thanks for the review, see comments inline.


> > Source/WebCore/html/HTMLElement.cpp:381
> > +	 ec = 0;
> I think this is generally the callers responsibility in WebCore.

Thanks, fixed.

> > Source/WebCore/html/HTMLElement.cpp:390
> > +		     fragment->appendChild(Text::create(document(),
text.substring(lineStart, i - lineStart)), ec);
>
> If this can run arbitrary javascript, "this" could get deleted, no?  Do we
need to suspend mutation events during this?

As the fragment isn't attached to the document yet I don't see how one would
listen to that mutation event. The text-node-append-data-remove-crash.html
tests this.

> > Source/WebCore/html/HTMLElement.cpp:399
> > +		 lineStart = i + 1;
> I find it difficult to read this loop and understand what its doing.	I can't
tell if that's a variable
> naming problem, the way the blocks are split up, or just my own
thick-headedness at this hour.

I agree, it's not the easiest code to read. I moved it out of the setInnerText
method and didn't want to make too many changes to it.

> > Source/WebCore/html/HTMLElement.cpp:498
> > +	     textPrev->appendData(textNode->data(), ec);
>
> Does this cause JS to run?  If so, our pointers could go invalid.

We hold RefPtrs for both nodes so it should be safe. This code has been
replaced with a call to mergeWithNextTextNode in the latest patch.

> > Source/WebCore/html/HTMLElement.cpp:511
> > +	     RefPtr<Text> textNext = static_cast<Text*>(next.get());
> > +	     RefPtr<Text> textNode = static_cast<Text*>(node);
> > +	     textNode->appendData(textNext->data(), ec);
> Seems we just did this above.  Maybe there is code to share here?

Good idea, broke out the merging logic into a helper funciton.