Bug 146038

Summary:

Position::findParent() should take a reference

Product:

WebKit

Reporter:

Jon Honeycutt <jhoneycutt>

Component:

HTML Editing

Assignee:

Jon Honeycutt <jhoneycutt>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, esprehn+autocc, kangil.han

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	darin: review+

Description Jon Honeycutt 2015-06-16 17:09:12 PDT

Position::findParent() should take a reference.

Comment 1 Jon Honeycutt 2015-06-16 17:10:31 PDT

Created attachment 254982 [details]
Patch

Comment 2 Darin Adler 2015-06-16 20:07:16 PDT

Comment on attachment 254982 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=254982&action=review

> Source/WebCore/dom/Position.cpp:165
> +        return findParent(*m_anchorNode.get());

No need for .get() here.

> Source/WebCore/dom/Position.cpp:225
> +        if (findParent(*m_anchorNode.get()) && (editingIgnoresContent(m_anchorNode.get()) || isRenderedTable(m_anchorNode.get())))

No need for .get() in the *m_anchorNode part.

> Source/WebCore/dom/Position.cpp:314
> +        if (!node)
> +            return *this;

Looks like this fixes a bug. Can we make a test that shows this was broken before?

> Source/WebCore/dom/Position.cpp:367
> +        if (!node)
> +            return *this;

Looks like this fixes a bug. Can we make a test that shows this was broken before?

Comment 3 Jon Honeycutt 2015-06-17 17:39:26 PDT

(In reply to comment #2)
> Comment on attachment 254982 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254982&action=review
> 
> > Source/WebCore/dom/Position.cpp:165
> > +        return findParent(*m_anchorNode.get());
> 
> No need for .get() here.
> 
> > Source/WebCore/dom/Position.cpp:225
> > +        if (findParent(*m_anchorNode.get()) && (editingIgnoresContent(m_anchorNode.get()) || isRenderedTable(m_anchorNode.get())))
> 
> No need for .get() in the *m_anchorNode part.

Fixed.

> 
> > Source/WebCore/dom/Position.cpp:314
> > +        if (!node)
> > +            return *this;
> 
> Looks like this fixes a bug. Can we make a test that shows this was broken
> before?
> 
> > Source/WebCore/dom/Position.cpp:367
> > +        if (!node)
> > +            return *this;
> 
> Looks like this fixes a bug. Can we make a test that shows this was broken
> before?

I couldn’t find a way to make this crash. I looked for existing bug reports and found <rdar://problem/21026135>, but I was unable to reproduce a crash given the repro steps in the bug.

Thanks for the review!

Comment 4 Jon Honeycutt 2015-06-17 17:50:33 PDT

Committed r185682: <http://trac.webkit.org/changeset/185682>