Summary: | Reduce PassRefPtr uses in editing | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||
Component: | WebCore Misc. | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Local Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Gyuyoung Kim
2016-03-21 23:07:55 PDT
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. |