Bug 155743 – Reduce PassRefPtr uses in editing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Gyuyoung Kim]

SSIA.

[Gyuyoung Kim]

Created attachment 274643 [details]
Patch

[Darin Adler]

Comment on attachment 274643 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Use RefPtr<>&& or raw pointer in arugment instaed of PassRefPtr.

Typo: instead

> Source/WebCore/editing/CompositeEditCommand.h:143
> +    void wrapContentsInDummySpan(Element*);

Should take Element&.

> Source/WebCore/editing/TextInsertionBaseCommand.h:43
> +    static void applyTextInsertionCommand(Frame*, TextInsertionBaseCommand*, const VisibleSelection& selectionForInsertion, const VisibleSelection& endingSelection);

Should take TextInsertionBaseCommand&.

> Source/WebCore/editing/TypingCommand.h:105
> +    static RefPtr<TypingCommand> lastTypingCommandIfStillOpenForTyping(Frame*);

Should take a Frame&.

> Source/WebCore/editing/WrapContentsInDummySpanCommand.h:37
> +    static Ref<WrapContentsInDummySpanCommand> create(Element* element)

Should take an Element&.

> Source/WebCore/editing/WrapContentsInDummySpanCommand.h:43
> +    explicit WrapContentsInDummySpanCommand(Element*);

Should take an Element&.

> Source/WebCore/editing/htmlediting.cpp:992
> +    return createTabSpanElement(document, RefPtr<Node>());

I think we should write nullptr or { } here instead of RefPtr<Node>().

> Source/WebCore/editing/markup.cpp:521
> +    return WTFMove(style);

Should not need WTFMove here because of the return value optimization.

> Source/WebCore/editing/markup.cpp:900
> +    return WTFMove(fragment);

Should not need WTFMove here because of the return value optimization.

> Source/WebCore/editing/markup.cpp:978
> +    return WTFMove(fragment);

Should not need WTFMove here because of the return value optimization.

> Source/WebCore/editing/markup.h:54
> +RefPtr<DocumentFragment> createFragmentForInnerOuterHTML(const String&, Element*, ParserContentPolicy, ExceptionCode&);

Should take Element&. That argument should be first, not after the string. Also seems that the String argument should be named.

> Source/WebCore/editing/markup.h:55
> +RefPtr<DocumentFragment> createFragmentForTransformToFragment(const String&, const String& sourceMIMEType, Document* outputDoc);

Should take Document&. Argument name “outputDoc” is not helpful, should be Document&, should be first argument like in createFragmentFromMarkup above. Also seems that the String argument should be named.

> Source/WebCore/editing/markup.h:56
> +RefPtr<DocumentFragment> createContextualFragment(const String&, HTMLElement*, ParserContentPolicy, ExceptionCode&);

Should take HTMLElement&. That argument should be first, not after the string. Also seems that the String argument should be named.

[Gyuyoung Kim]

Created attachment 274727 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #2)
> Comment on attachment 274643 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274643&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Use RefPtr<>&& or raw pointer in arugment instaed of PassRefPtr.
> 
> Typo: instead
> 
> > Source/WebCore/editing/CompositeEditCommand.h:143
> > +    void wrapContentsInDummySpan(Element*);
> 
> Should take Element&.
> 
> > Source/WebCore/editing/TextInsertionBaseCommand.h:43
> > +    static void applyTextInsertionCommand(Frame*, TextInsertionBaseCommand*, const VisibleSelection& selectionForInsertion, const VisibleSelection& endingSelection);
> 
> Should take TextInsertionBaseCommand&.
> 
> > Source/WebCore/editing/TypingCommand.h:105
> > +    static RefPtr<TypingCommand> lastTypingCommandIfStillOpenForTyping(Frame*);
> 
> Should take a Frame&.
> 
> > Source/WebCore/editing/WrapContentsInDummySpanCommand.h:37
> > +    static Ref<WrapContentsInDummySpanCommand> create(Element* element)
> 
> Should take an Element&.
> 
> > Source/WebCore/editing/WrapContentsInDummySpanCommand.h:43
> > +    explicit WrapContentsInDummySpanCommand(Element*);
> 
> Should take an Element&.

> > Source/WebCore/editing/markup.cpp:521
> > +    return WTFMove(style);
> 
> Should not need WTFMove here because of the return value optimization.
> 
> > Source/WebCore/editing/markup.cpp:900
> > +    return WTFMove(fragment);
> 
> Should not need WTFMove here because of the return value optimization.
> 
> > Source/WebCore/editing/markup.cpp:978
> > +    return WTFMove(fragment);
> 
> Should not need WTFMove here because of the return value optimization.
> 
> > Source/WebCore/editing/markup.h:54
> > +RefPtr<DocumentFragment> createFragmentForInnerOuterHTML(const String&, Element*, ParserContentPolicy, ExceptionCode&);
> 
> Should take Element&. That argument should be first, not after the string.
> Also seems that the String argument should be named.
> 
> > Source/WebCore/editing/markup.h:55
> > +RefPtr<DocumentFragment> createFragmentForTransformToFragment(const String&, const String& sourceMIMEType, Document* outputDoc);
> 
> Should take Document&. Argument name “outputDoc” is not helpful, should be
> Document&, should be first argument like in createFragmentFromMarkup above.
> Also seems that the String argument should be named.
> 
> > Source/WebCore/editing/markup.h:56
> > +RefPtr<DocumentFragment> createContextualFragment(const String&, HTMLElement*, ParserContentPolicy, ExceptionCode&);
> 
> Should take HTMLElement&. That argument should be first, not after the
> string. Also seems that the String argument should be named.

Thank you for your detailed review ! Fixed.

[Gyuyoung Kim]

Created attachment 274739 [details]
Patch

[Gyuyoung Kim]

Created attachment 274742 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 274742 [details]
Patch

Clearing flags on attachment: 274742

Committed r198583: <http://trac.webkit.org/changeset/198583>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Comment on attachment 274742 [details]
Patch

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

> Source/WebCore/editing/VisibleSelection.cpp:223
> +    return WTFMove(searchRange);

This is not right - moving the result is bad for performance, as it prevents copy elision. Some versions of compilers warn about that, breaking the build.

Fixed in r198587.

[Gyuyoung Kim]

(In reply to comment #9)
> Comment on attachment 274742 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274742&action=review
> 
> > Source/WebCore/editing/VisibleSelection.cpp:223
> > +    return WTFMove(searchRange);
> 
> This is not right - moving the result is bad for performance, as it prevents
> copy elision. Some versions of compilers warn about that, breaking the build.
> 
> Fixed in r198587.

Thanks for fixing it.