Purge all uses of PassRefPtr in DOM - 1

• Component: DOM
• Status: RESOLVED FIXED

[Gyuyoung Kim]

There are too many uses of PassRefPtr in WebCore/dom. So it will be removed step by step.

[Gyuyoung Kim]

Created attachment 262387 [details]
Patch

[WebKit Commit Bot]

Attachment 262387 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DeviceMotionData.cpp:85:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:84:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:85:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:86:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:87:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.h:86:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.h:97:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 62 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Created attachment 262389 [details]
Patch

[WebKit Commit Bot]

Attachment 262389 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DeviceMotionData.cpp:85:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:84:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:85:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:86:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:87:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.h:86:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.h:97:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 61 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 262389 [details]
Patch

Attachment 262389 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/242403

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 262390 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Gyuyoung Kim]

Created attachment 262391 [details]
Patch

[WebKit Commit Bot]

Attachment 262391 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DeviceMotionData.cpp:85:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:84:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:85:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:86:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:87:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.h:86:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.h:97:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 262391 [details]
Patch

Attachment 262391 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/242540

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 262394 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 262391 [details]
Patch

Attachment 262391 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/242566

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 262396 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Gyuyoung Kim]

Created attachment 263133 [details]
Patch

[WebKit Commit Bot]

Attachment 263133 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DeviceMotionData.cpp:85:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:84:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:85:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:86:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:87:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.h:86:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.h:97:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 63 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Created attachment 263137 [details]
Patch

[WebKit Commit Bot]

Attachment 263137 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DeviceMotionData.cpp:85:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:84:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:85:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:86:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:87:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.h:86:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.h:97:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 64 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Created attachment 263138 [details]
Patch

[WebKit Commit Bot]

Attachment 263138 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/DeviceMotionData.cpp:85:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:72:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:84:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:85:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:86:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.cpp:87:  Wrong number of spaces before statement. (expected: 39)  [whitespace/indent] [4]
ERROR: Source/WebCore/dom/DeviceMotionData.h:86:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/dom/DeviceMotionData.h:97:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 8 in 65 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

I see a number of small issues, not exactly mistakes but design problems that already exist in the code that we are possibly making a bit worse, but I do not have time to do the entire review right now.

[Darin Adler]

Comment on attachment 263138 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263138&action=review

All those dispatchEvent functions should probably just take Event&; there is no code in there that takes ownership of the event.

I reviewed as much of this as I could, but there are a lot of cases here that are wrong and I ran out of steam before I got to the end of the patch. We were using PassRefPtr in many cases where it simply was not the appropriate type, and converting to RefPtr&& is not really the right thing to do in all those cases.

RefPtr&& should mostly only be used when there is at least one caller who is actually going to pass ownership in, and when the implementation optimize the case where it actually takes ownership. And where null is a valid value.

If null is not valid, but it otherwise meets the description above, then it should be Ref&&.

If callers aren’t in a position to pass ownership or the receiver isn’t in a position to take ownership in an optimized way, then we should use plain old references or pointers instead.

> Source/WebCore/dom/ChildListMutationScope.cpp:74
> +    return WTF::move(accumulator);

Normally we don’t need a WTF::move on a return value like this; the return value optimization takes care of it.

> Source/WebCore/dom/ChildListMutationScope.h:49
> +    static RefPtr<ChildListMutationAccumulator> getOrCreate(ContainerNode&);

Should return Ref, not RefPtr; this never returns null.

> Source/WebCore/dom/CompositionEvent.h:47
> -    static Ref<CompositionEvent> create(const AtomicString& type, PassRefPtr<AbstractView> view, const String& data)
> +    static Ref<CompositionEvent> create(const AtomicString& type, RefPtr<AbstractView>&& view, const String& data)

This should just take AbstractView&; we are never passing in a newly created object here and so move semantics aren’t helpful. Same applies for initCompositionEvent and the actual constructor. Although for initCompositionEvent we might have to take a pointer than a reference depending on how functions called by the bindings generator.

> Source/WebCore/dom/ContainerNode.cpp:726
> -        // FIXME: This method should take a PassRefPtr.
> +        // FIXME: This method should take a RefPtr&&.

I’m not sure this comment makes sense any more. If appendChildCommon took a RefPtr&& it would do us no good, because we call adoptIfNeeded *after* calling it.

> Source/WebCore/dom/DataTransfer.h:71
> -        PassRefPtr<DataTransferItemList> items() = 0;
> +        RefPtr<DataTransferItemList> items() = 0;

I don’t understand what’s going on here. We are changing the types on these pure virtual functions, but I don’t see us changing the types on the function implementations in concrete derived classes. What concrete class derives from this? Is this dead code?

> Source/WebCore/dom/DataTransferItem.h:57
> -    virtual void getAsString(PassRefPtr<StringCallback>) const = 0;
> -    virtual PassRefPtr<Blob> getAsFile() const = 0;
> +    virtual void getAsString(RefPtr<StringCallback>&&) const = 0;
> +    virtual RefPtr<Blob> getAsFile() const = 0;

The type here should probably just be StringCallback& or StringCallback*. There is no path where the bindings passes ownership of a RefPtr to the function it calls, so this does not optimize anything.

But also, I don’t understand what’s going on here. We are changing the types on these pure virtual functions, but I don’t see us changing the types on the function implementations in concrete derived classes. What concrete class derives from this? Is this dead code?

> Source/WebCore/dom/DataTransferItemList.h:50
>      virtual size_t length() const = 0;

The type here is wrong. It should be unsigned, not size_t. Not new to this patch.

> Source/WebCore/dom/DataTransferItemList.h:52
> +    virtual RefPtr<DataTransferItem> item(unsigned long index) = 0;
>      virtual void deleteItem(unsigned long index, ExceptionCode&) = 0;

The types here are wrong. They should be unsigned, not unsigned long. Not new to this patch.

But also, I don’t understand what’s going on here. We are changing the types on these pure virtual functions, but I don’t see us changing the types on the function implementations in concrete derived classes. What concrete class derives from this? Is this dead code?

> Source/WebCore/dom/DataTransferItemList.h:55
> -    virtual void add(PassRefPtr<File>) = 0;
> +    virtual void add(RefPtr<File>&&) = 0;

The type here should probably just be File& or File*. There is no path where the bindings passes ownership of a RefPtr to the function it calls, so this does not optimize anything.

But also, I don’t understand what’s going on here. We are changing the types on these pure virtual functions, but I don’t see us changing the types on the function implementations in concrete derived classes. What concrete class derives from this? Is this dead code?

> Source/WebCore/dom/DeviceMotionController.h:55
> +    virtual RefPtr<Event> getLastEvent() override;

Should be Ref, not RefPtr.

> Source/WebCore/dom/DeviceMotionData.h:86
> +    WEBCORE_EXPORT static Ref<DeviceMotionData> create(RefPtr<Acceleration>&&, RefPtr<Acceleration>&& accelerationIncludingGravity,
> +                                                       RefPtr<RotationRate>&&, bool canProvideInterval, double interval);

Do callers actually move in these objects? And is it legal for them to be null? These probably should be references, pointers, or Ref&&, not necessarily RefPtr&&.

Also, WebKit style guide specifically states we should not do this kind of formatting where we line up one line with the next. Instead this could be one long line, or could be indented four spaces.

> Source/WebCore/dom/DeviceMotionData.h:97
> +    DeviceMotionData(RefPtr<Acceleration>&&, RefPtr<Acceleration>&& accelerationIncludingGravity,
> +                     RefPtr<RotationRate>&&, bool canProvideInterval, double interval);

Ditto.

> Source/WebCore/dom/DeviceOrientationController.h:56
> +    virtual RefPtr<Event> getLastEvent() override;

Should be Ref, not RefPtr.

> Source/WebCore/dom/Document.h:441
> +    void setInputCursor(RefPtr<JSC::InputCursor>&&);

This should just take JSC::InputCursor& or possibly JSC::InputCursor* depending on how bindings work; we are never passing in a newly created object here and so move semantics aren’t helpful.

> Source/WebCore/dom/Document.h:455
> +    RefPtr<Node> adoptNode(RefPtr<Node>&& source, ExceptionCode&);

This should just take Node& or possibly Node* depending on how bindings work; we are never passing in a newly created object here and so move semantics aren’t helpful.

> Source/WebCore/dom/Document.h:717
> +    WEBCORE_EXPORT bool setFocusedElement(RefPtr<Element>&&, FocusDirection = FocusDirectionNone);

This should just take Element& or possibly Element* depending on how bindings work; we are never passing in a newly created object here and so move semantics aren’t helpful.

> Source/WebCore/dom/Document.h:780
> +    WEBCORE_EXPORT void dispatchWindowEvent(RefPtr<Event>&&, RefPtr<EventTarget>&& = 0);

This should just take Event& or possibly Event* depending on how bindings work and EventTarget*; we are never passing in a newly created object here and so move semantics aren’t helpful.

Should be nullptr, not 0.

> Source/WebCore/dom/Document.h:908
> +    void setBodyOrFrameset(RefPtr<HTMLElement>&&, ExceptionCode&);

This should just take HTMLElement& or possibly HTMLElement* depending on how bindings work; we are never passing in a newly created object here and so move semantics aren’t helpful.

> Source/WebCore/dom/Document.h:941
> +    void pushCurrentScript(RefPtr<HTMLScriptElement>&&);

This should just take HTMLScriptElement& or possibly HTMLScriptElement*; we are never passing in a newly created object here and so move semantics aren’t helpful.

> Source/WebCore/dom/Document.h:1040
> +    void setDecoder(RefPtr<TextResourceDecoder>&&);

Not certain this should be RefPtr&&. Do callers pass a newly created object? Can it be null? It’s possible this should be a pointer, reference, or Ref&&.

> Source/WebCore/dom/Document.h:1044
> -    RefPtr<StringImpl> displayStringModifiedByEncoding(PassRefPtr<StringImpl>) const;
> +    RefPtr<StringImpl> displayStringModifiedByEncoding(RefPtr<StringImpl>&&) const;

We should probably just delete this function. Where is it used?

> Source/WebCore/dom/Document.h:1076
> -    void statePopped(PassRefPtr<SerializedScriptValue>);
> +    void statePopped(RefPtr<SerializedScriptValue>&&);

Not sure if this really is passing ownership or if it can ever be null. Should probably be SerializedScriptValue& or maybe a pointer or Ref&&.

> Source/WebCore/dom/Document.h:1086
> +    void enqueueWindowEvent(RefPtr<Event>&&);
> +    void enqueueDocumentEvent(RefPtr<Event>&&);
> +    void enqueueOverflowEvent(RefPtr<Event>&&);

These should all be Ref<Event>&&, not RefPtr<Event>&&.

> Source/WebCore/dom/Document.h:1089
> +    void enqueuePopstateEvent(RefPtr<SerializedScriptValue>&& stateObject);

This should be Ref<SerializedScriptValue>&&, not RefPtr. Also no need for an argument name here.

> Source/WebCore/dom/Document.h:1157
> +    int requestAnimationFrame(RefPtr<RequestAnimationFrameCallback>&&);

This should just take RequestAnimationFrameCallback& or possibly RequestAnimationFrameCallback* depending on how bindings work; we are never passing in a newly created object here and so move semantics aren’t helpful.

> Source/WebCore/dom/DocumentEventQueue.h:47
> +    virtual bool enqueueEvent(RefPtr<Event>&&) override;

This should be Ref<Event>&&, not RefPtr.

> Source/WebCore/dom/DocumentMarker.cpp:71
> +    static RefPtr<DocumentMarkerTextMatch> instanceFor(bool);

This should return DocumentMarkerTextMatch&; it never creates an object.

> Source/WebCore/dom/DocumentMarker.h:123
> +    DocumentMarker(MarkerType, unsigned startOffset, unsigned endOffset, RefPtr<DocumentMarkerDetails>&&);

Can this ever be null? I think maybe it should be a Ref&& instead.

> Source/WebCore/dom/DocumentMarkerController.h:56
> +    void addMarkerToNode(Node*, unsigned startOffset, unsigned length, DocumentMarker::MarkerType, RefPtr<DocumentMarkerDetails>&&);

Can this ever be null? I think maybe it should be a Ref&& instead.

> Source/WebCore/dom/Event.h:109
> +    void setTarget(RefPtr<EventTarget>&&);

This should just be EventTarget*; no one is passing ownership here.

> Source/WebCore/dom/Event.h:172
> +    void setUnderlyingEvent(RefPtr<Event>&&);

This should just be Event*; no one is passing ownership here.

> Source/WebCore/dom/Event.h:178
> +    virtual RefPtr<Event> cloneFor(HTMLIFrameElement*) const;

Should return Ref<Event>; it can’t ever return null.

[Gyuyoung Kim]

Darin, thank you for your detail review. It looks current patch should be fully modified again. I will prepare more small patch to land this patch safely.

[Gyuyoung Kim]

Created attachment 265145 [details]
Patch

[Gyuyoung Kim]

Sorry for too late update. I try to remove *PassRefPtr* in obvious places first to close this bug. Remained PassRefPtr will be removed in following bugs again.

[Darin Adler]

Comment on attachment 265145 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265145&action=review

> Source/WebCore/dom/ChildListMutationScope.h:49
> +    static RefPtr<ChildListMutationAccumulator> getOrCreate(ContainerNode&);

Return value for this should be Ref. It can never return a null pointer.

> Source/WebCore/dom/ClipboardEvent.h:41
> +        static Ref<ClipboardEvent> create(const AtomicString& type, bool canBubbleArg, bool cancelableArg, RefPtr<DataTransfer>&& clipboardArg)

I don’t think we ever get any benefit out of moving in a DataTransfer. So we should change this to just be a DataTransfer* or a DataTransfer& depending on whether it can ever be null.

> Source/WebCore/dom/CompositionEvent.h:47
> +    static Ref<CompositionEvent> create(const AtomicString& type, RefPtr<AbstractView>&& view, const String& data)

There’s no value to the move semantics for AbstractView. When called by JavaScript, the AbstractView is going to be owned by the wrapper, so there is no existing RefPtr to hand off ownership of. When called from inside the C++ DOM, it’s just going to be a Document& or Document*, and again there is no ownership to hand off. So all these arguments should be changed to either AbstractView* or AbstractView&, depending on whether it’s OK to be null. That’s true for all the event classes: the create functions, init functions, and constructors. (It’s possible the init function argument might need to be a pointer even if the others can be references. Should double check we have a test case for that.)

[Gyuyoung Kim]

Comment on attachment 265145 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265145&action=review

>> Source/WebCore/dom/ChildListMutationScope.h:49
>> +    static RefPtr<ChildListMutationAccumulator> getOrCreate(ContainerNode&);
> 
> Return value for this should be Ref. It can never return a null pointer.

In below getOrCreate() it looks accumulator can be nullptr, because first line sets initial value to nullptr, isn't it ?


typedef HashMap<ContainerNode*, ChildListMutationAccumulator*> AccumulatorMap;
RefPtr<ChildListMutationAccumulator> ChildListMutationAccumulator::getOrCreate(ContainerNode& target)
{
    AccumulatorMap::AddResult result = accumulatorMap().add(&target, nullptr);
    RefPtr<ChildListMutationAccumulator> accumulator;
    if (!result.isNewEntry)
        accumulator = result.iterator->value;
    else {

>> Source/WebCore/dom/CompositionEvent.h:47
>> +    static Ref<CompositionEvent> create(const AtomicString& type, RefPtr<AbstractView>&& view, const String& data)
> 
> There’s no value to the move semantics for AbstractView. When called by JavaScript, the AbstractView is going to be owned by the wrapper, so there is no existing RefPtr to hand off ownership of. When called from inside the C++ DOM, it’s just going to be a Document& or Document*, and again there is no ownership to hand off. So all these arguments should be changed to either AbstractView* or AbstractView&, depending on whether it’s OK to be null. That’s true for all the event classes: the create functions, init functions, and constructors. (It’s possible the init function argument might need to be a pointer even if the others can be references. Should double check we have a test case for that.)

Thank you for your deep insight. Let me try to keep this comment for upcoming patches. Anyway change it with a pointer.

[Gyuyoung Kim]

Created attachment 265264 [details]
Patch

[Darin Adler]

Comment on attachment 265145 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=265145&action=review

>>> Source/WebCore/dom/ChildListMutationScope.h:49
>>> +    static RefPtr<ChildListMutationAccumulator> getOrCreate(ContainerNode&);
>> 
>> Return value for this should be Ref. It can never return a null pointer.
> 
> In below getOrCreate() it looks accumulator can be nullptr, because first line sets initial value to nullptr, isn't it ?
> 
> 
> typedef HashMap<ContainerNode*, ChildListMutationAccumulator*> AccumulatorMap;
> RefPtr<ChildListMutationAccumulator> ChildListMutationAccumulator::getOrCreate(ContainerNode& target)
> {
>     AccumulatorMap::AddResult result = accumulatorMap().add(&target, nullptr);
>     RefPtr<ChildListMutationAccumulator> accumulator;
>     if (!result.isNewEntry)
>         accumulator = result.iterator->value;
>     else {

No, that nullptr value passed into add is only a placeholder. Whenever it’s used, isNewEntry will be true, and so the function will allocate a new object and store it, overwriting the nullptr.

[Gyuyoung Kim]

Created attachment 266585 [details]
Patch for landing

[Gyuyoung Kim]

(In reply to comment #27)
> Comment on attachment 265145 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=265145&action=review
> 
> >>> Source/WebCore/dom/ChildListMutationScope.h:49
> >>> +    static RefPtr<ChildListMutationAccumulator> getOrCreate(ContainerNode&);
> >> 
> >> Return value for this should be Ref. It can never return a null pointer.
> > 
> > In below getOrCreate() it looks accumulator can be nullptr, because first line sets initial value to nullptr, isn't it ?
> > 
> > 
> > typedef HashMap<ContainerNode*, ChildListMutationAccumulator*> AccumulatorMap;
> > RefPtr<ChildListMutationAccumulator> ChildListMutationAccumulator::getOrCreate(ContainerNode& target)
> > {
> >     AccumulatorMap::AddResult result = accumulatorMap().add(&target, nullptr);
> >     RefPtr<ChildListMutationAccumulator> accumulator;
> >     if (!result.isNewEntry)
> >         accumulator = result.iterator->value;
> >     else {
> 
> No, that nullptr value passed into add is only a placeholder. Whenever it’s
> used, isNewEntry will be true, and so the function will allocate a new
> object and store it, overwriting the nullptr.

Thanks. However I need to have time to fix it. I'd like to handle it in following patches again.

[WebKit Commit Bot]

Comment on attachment 266585 [details]
Patch for landing

Clearing flags on attachment: 266585

Committed r193408: <http://trac.webkit.org/changeset/193408>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.