[Webkit-unassigned] [Bug 52268] Setting outerText should convert CR/LF to <br>

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=52268


Emil A Eklund <eae at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #78735|                            |review?
               Flag|                            |




--- Comment #5 from Emil A Eklund <eae at chromium.org>  2011-01-12 14:52:29 PST ---
Created an attachment (id=78735)
 --> (https://bugs.webkit.org/attachment.cgi?id=78735&action=review)
Patch

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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.