SSIA.
Created attachment 274643 [details] Patch
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.
Created attachment 274727 [details] Patch
(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.
Created attachment 274739 [details] Patch
Created attachment 274742 [details] Patch
Comment on attachment 274742 [details] Patch Clearing flags on attachment: 274742 Committed r198583: <http://trac.webkit.org/changeset/198583>
All reviewed patches have been landed. Closing bug.
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.
(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.