Bug 153055 – Reduce PassRefPtr uses in dom - 3

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

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 268853 [details]
Patch

[Build Bot]

Comment on attachment 268853 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 268857 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

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

[Build Bot]

Comment on attachment 268853 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 268858 [details]
Archive of layout-test-results from ews101 for mac-yosemite

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

[Build Bot]

Comment on attachment 268853 [details]
Patch

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

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 268859 [details]
Archive of layout-test-results from ews114 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Gyuyoung Kim]

Created attachment 268930 [details]
Patch

[Alex Christensen]

Comment on attachment 268930 [details]
Patch

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

> Source/WebCore/dom/MutationObserverInterestGroup.cpp:81
> -                mutationWithNullOldValue = MutationRecord::createWithNullOldValue(mutation).get();
> +                mutationWithNullOldValue = MutationRecord::createWithNullOldValue(*mutation).ptr();

This is not more unsafe than the old code.  I assume prpMutation will become a Ref<MutationRecord> instead of a PassRefPtr<MutationRecord>.  Why not do that change, too, in this patch?  The we wouldn't be adding another unchecked null dereference.

[Gyuyoung Kim]

(In reply to comment #9)
> Comment on attachment 268930 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268930&action=review
> 
> > Source/WebCore/dom/MutationObserverInterestGroup.cpp:81
> > -                mutationWithNullOldValue = MutationRecord::createWithNullOldValue(mutation).get();
> > +                mutationWithNullOldValue = MutationRecord::createWithNullOldValue(*mutation).ptr();
> 
> This is not more unsafe than the old code.  I assume prpMutation will become
> a Ref<MutationRecord> instead of a PassRefPtr<MutationRecord>.  Why not do
> that change, too, in this patch?  The we wouldn't be adding another
> unchecked null dereference.

When I tried to change it, patch is bigger than now. Because big change is able to sometime cause unexpected crash, I postponed it to next patch. However it is cool to change it in this patch together, let me try it.

[Darin Adler]

Comment on attachment 268930 [details]
Patch

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

> Source/WebCore/dom/ShadowRoot.h:84
> +    RefPtr<Node> cloneNode(bool, ExceptionCode&);

After studying the code, I realize that this function is dead code, never called. The right thing to do in this patch is to delete this function entirely.

Separately, someone needs to make a test case that tries to clone a shadow root. I expect they will find that no exception is raised, and we will need code changes to make that happen. But this function won’t do the trick.

[Gyuyoung Kim]

Created attachment 269247 [details]
Patch for landing

[Gyuyoung Kim]

(In reply to comment #11)
> Comment on attachment 268930 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=268930&action=review
> 
> > Source/WebCore/dom/ShadowRoot.h:84
> > +    RefPtr<Node> cloneNode(bool, ExceptionCode&);
> 
> After studying the code, I realize that this function is dead code, never
> called. The right thing to do in this patch is to delete this function
> entirely.

Thanks, removed.

[WebKit Commit Bot]

Comment on attachment 269247 [details]
Patch for landing

Clearing flags on attachment: 269247

Committed r195243: <http://trac.webkit.org/changeset/195243>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.