Summary: | Reduce PassRefPtr uses in dom - 3 | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||
Component: | DOM | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | buildbot, cdumez, commit-queue, darin, esprehn+autocc, kangil.han, rniwa, WebkitBugTracker | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | WebKit Local Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 144092 | ||||||||||||||||
Attachments: |
|
Description
Gyuyoung Kim
2016-01-12 21:08:48 PST
Created attachment 268853 [details]
Patch
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. 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
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. 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
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. 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
Created attachment 268930 [details]
Patch
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. (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. 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. Created attachment 269247 [details]
Patch for landing
(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. Comment on attachment 269247 [details] Patch for landing Clearing flags on attachment: 269247 Committed r195243: <http://trac.webkit.org/changeset/195243> All reviewed patches have been landed. Closing bug. |