Bug 146038 - Position::findParent() should take a reference
Summary: Position::findParent() should take a reference
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jon Honeycutt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-06-16 17:09 PDT by Jon Honeycutt
Modified: 2015-06-17 17:50 PDT (History)
3 users (show)

See Also:

Attachments
Patch (5.01 KB, patch)
2015-06-16 17:10 PDT, Jon Honeycutt 		darin: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>