SSIA
Created attachment 269842 [details] Patch
Created attachment 269868 [details] Patch
Created attachment 269869 [details] Patch
Created attachment 269879 [details] Patch
Comment on attachment 269879 [details] Patch It looks windows fail wasn't caused by this patch.
Comment on attachment 269879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269879&action=review > Source/WebCore/dom/Document.h:469 > + RefPtr<Node> adoptNode(RefPtr<Node>&& source, ExceptionCode&); Do any callers pass a newly-created object to this? Please note that the JavaScript bindings will never do this, so it has to be some call site in native code. I’m pretty sure the answer is no, so I’m pretty sure the type here should just be Node*. > Source/WebCore/dom/DocumentMarker.cpp:69 > class DocumentMarkerTextMatch : public DocumentMarkerDetails { This class should probably be marked final. > Source/WebCore/dom/DocumentMarker.cpp:87 > - static DocumentMarkerTextMatch* trueInstance = adoptRef(new DocumentMarkerTextMatch(true)).leakRef(); > - static DocumentMarkerTextMatch* falseInstance = adoptRef(new DocumentMarkerTextMatch(false)).leakRef(); > - return match ? trueInstance : falseInstance; > + return match ? adoptRef(*new DocumentMarkerTextMatch(true)) : adoptRef(*new DocumentMarkerTextMatch(false)); This is a peculiar change. Before, this function always returned an existing object. Now we are creating a new object every time; that will almost certainly make the code slower. Not sure why we decided to remove this optimization. Also, the new code uses ? : in a strange way; could just pass match in. The old code was written that way because it had two global instances. I suggest the return type of this function be DocumentMarkerTextMatch& instead of Ref<DocumentMarkerTextMatch> since it returns existing objects and doesn’t create new ones. > Source/WebCore/dom/Event.cpp:179 > +void Event::setUnderlyingEvent(Event* ue) Since we are touching this code we should expand "ue" to "underlyingEvent" and "e" to "event". > Source/WebCore/dom/TextEvent.cpp:52 > + return adoptRef(*new TextEvent(view, "", WTFMove(data), shouldSmartReplace, shouldMatchStyle, mailBlockquoteHandling)); This should use emptyString() instead of "".
Created attachment 270303 [details] Patch
Comment on attachment 269879 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=269879&action=review >> Source/WebCore/dom/DocumentMarker.cpp:87 >> + return match ? adoptRef(*new DocumentMarkerTextMatch(true)) : adoptRef(*new DocumentMarkerTextMatch(false)); > > This is a peculiar change. > > Before, this function always returned an existing object. Now we are creating a new object every time; that will almost certainly make the code slower. Not sure why we decided to remove this optimization. Also, the new code uses ? : in a strange way; could just pass match in. The old code was written that way because it had two global instances. > > I suggest the return type of this function be DocumentMarkerTextMatch& instead of Ref<DocumentMarkerTextMatch> since it returns existing objects and doesn’t create new ones. I missed this is *static* variables. However it looks I have to modify more files in order to change Ref<DocumentMarkerTextMatch> with DocumentMarkerTextMatch& because build breaks happen on callers. I'd like to postpone this fix in next clean-up. Other comments are fixed.
Comment on attachment 270303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=270303&action=review > Source/WebCore/editing/Editor.h:349 > + WEBCORE_EXPORT void pasteAsFragment(RefPtr<DocumentFragment>&&, bool smartReplace, bool matchStyle, MailBlockquoteHandling = MailBlockquoteHandling::RespectBlockquote); This should be Ref. All the call sites are already checking for null before they call this.
Created attachment 270335 [details] Patch for landing
(In reply to comment #9) > Comment on attachment 270303 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=270303&action=review > > > Source/WebCore/editing/Editor.h:349 > > + WEBCORE_EXPORT void pasteAsFragment(RefPtr<DocumentFragment>&&, bool smartReplace, bool matchStyle, MailBlockquoteHandling = MailBlockquoteHandling::RespectBlockquote); > > This should be Ref. All the call sites are already checking for null before > they call this. Ok, but call sites need to use releaseNonNull() instead of WTFMove() if we use Ref.
Comment on attachment 270335 [details] Patch for landing Clearing flags on attachment: 270335 Committed r195927: <http://trac.webkit.org/changeset/195927>
All reviewed patches have been landed. Closing bug.