Bug 153470 - Reduce PassRefPtr uses in dom - 5
Summary: Reduce PassRefPtr uses in dom - 5
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gyuyoung Kim
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-25 19:06 PST by Gyuyoung Kim
Modified: 2016-01-31 04:08 PST (History)
1 user (show)

See Also:


Attachments
Patch (22.16 KB, patch)
2016-01-25 19:10 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (23.94 KB, patch)
2016-01-26 01:44 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (25.40 KB, patch)
2016-01-26 01:45 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (26.64 KB, patch)
2016-01-26 05:59 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch (26.16 KB, patch)
2016-01-29 23:43 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff
Patch for landing (27.12 KB, patch)
2016-01-30 20:30 PST, Gyuyoung Kim
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gyuyoung Kim 2016-01-25 19:06:14 PST
SSIA
Comment 1 Gyuyoung Kim 2016-01-25 19:10:12 PST
Created attachment 269842 [details]
Patch
Comment 2 Gyuyoung Kim 2016-01-26 01:44:12 PST
Created attachment 269868 [details]
Patch
Comment 3 Gyuyoung Kim 2016-01-26 01:45:57 PST
Created attachment 269869 [details]
Patch
Comment 4 Gyuyoung Kim 2016-01-26 05:59:12 PST
Created attachment 269879 [details]
Patch
Comment 5 Gyuyoung Kim 2016-01-27 17:37:24 PST
Comment on attachment 269879 [details]
Patch

It looks windows fail wasn't caused by this patch.
Comment 6 Darin Adler 2016-01-28 09:07:06 PST
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 "".
Comment 7 Gyuyoung Kim 2016-01-29 23:43:00 PST
Created attachment 270303 [details]
Patch
Comment 8 Gyuyoung Kim 2016-01-30 00:47:15 PST
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 9 Darin Adler 2016-01-30 11:27:08 PST
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.
Comment 10 Gyuyoung Kim 2016-01-30 20:30:18 PST
Created attachment 270335 [details]
Patch for landing
Comment 11 Gyuyoung Kim 2016-01-30 20:32:15 PST
(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 12 WebKit Commit Bot 2016-01-31 04:08:39 PST
Comment on attachment 270335 [details]
Patch for landing

Clearing flags on attachment: 270335

Committed r195927: <http://trac.webkit.org/changeset/195927>
Comment 13 WebKit Commit Bot 2016-01-31 04:08:44 PST
All reviewed patches have been landed.  Closing bug.