Bug 153470 – Reduce PassRefPtr uses in dom - 5

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 269842 [details]
Patch

[Gyuyoung Kim]

Created attachment 269868 [details]
Patch

[Gyuyoung Kim]

Created attachment 269869 [details]
Patch

[Gyuyoung Kim]

Created attachment 269879 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 269879 [details]
Patch

It looks windows fail wasn't caused by this patch.

[Darin Adler]

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 "".

[Gyuyoung Kim]

Created attachment 270303 [details]
Patch

[Gyuyoung Kim]

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.

[Darin Adler]

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.

[Gyuyoung Kim]

Created attachment 270335 [details]
Patch for landing

[Gyuyoung Kim]

(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.

[WebKit Commit Bot]

Comment on attachment 270335 [details]
Patch for landing

Clearing flags on attachment: 270335

Committed r195927: <http://trac.webkit.org/changeset/195927>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.