Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2572423002:
[SPv2] Use PassRefPtr in PropertyTreeState setter and remove meaningless DCHECK (Closed)
|
Created:
4 years ago by trchen Modified:
4 years ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), Justin Novosad, pdr+graphicswatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[SPv2] Use PassRefPtr in PropertyTreeState setter and remove meaningless DCHECK
The common pattern is to accept PassRefPtr in setters because it will allow
zero cost ownership transfer. Also the DCHECK for non-unique ownership is
removed, because it never worked as intended.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Committed: https://crrev.com/078212bbd956d7d72333d75ca3c2d2da5fff1190
Cr-Commit-Position: refs/heads/master@{#439626}
Patch Set 1 #
Total comments: 1
Patch Set 2 : PassRefPtr<T> ---> RefPtr<T>&& #
Total comments: 1
Patch Set 3 : that makes sense. thanks! #Messages
Total messages: 26 (16 generated)
Description was changed from ========== [SPv2] Use PassRefPtr in PropertyTreeState setter and remove meaningless DCHECK The common pattern is to accept PassRefPtr in setters because it will allow zero cost ownership transfer. Also the DCHECK for non-unique ownership is removed, because it never worked as intended. ========== to ========== [SPv2] Use PassRefPtr in PropertyTreeState setter and remove meaningless DCHECK The common pattern is to accept PassRefPtr in setters because it will allow zero cost ownership transfer. Also the DCHECK for non-unique ownership is removed, because it never worked as intended. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
trchen@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/2572423002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h (left): https://codereview.chromium.org/2572423002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h:43: DCHECK(!node->hasOneRef()); Just realized this DCHECK makes no sense. The only way 'node' could have exactly 1 reference at this point is that m_transform.get() == node in the beginning. Otherwise assigning to m_transform increases reference count by 1, and the reference count should already be 1 or more in the beginning.
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM
FYI, there was a PSA that PassRefPtr is deprecated. https://bugs.chromium.org/p/chromium/issues/detail?id=494719 https://groups.google.com/a/chromium.org/d/msg/blink-dev/h21zh25fvpE/DUxEYWFW... Though I haven't seen activity on that in >1 month, so not sure what's going on.
trchen@chromium.org changed reviewers: + jbroman@chromium.org
Thanks for the heads up! Could you double check I'm doing the pattern right?
The CQ bit was checked by trchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2572423002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h (right): https://codereview.chromium.org/2572423002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PropertyTreeState.h:41: void setTransform(RefPtr<const TransformPaintPropertyNode>&& node) { Prefer RefPtr<T> as the argument, and std::move to transfer ownership to another RefPtr. void setTransform(RefPtr<const TransformPaintPropertyNode> node) { m_transform = std::move(node); }
The CQ bit was checked by trchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/2572423002/#ps40001 (title: "that makes sense. thanks!")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by trchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482184787356010,
"parent_rev": "493553cb04a708709faf2cd43a39b1956b9027f9", "commit_rev":
"25f7128b3419b9d24b26e0d8169fa30036e6e6a9"}
Message was sent while issue was closed.
Description was changed from ========== [SPv2] Use PassRefPtr in PropertyTreeState setter and remove meaningless DCHECK The common pattern is to accept PassRefPtr in setters because it will allow zero cost ownership transfer. Also the DCHECK for non-unique ownership is removed, because it never worked as intended. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPv2] Use PassRefPtr in PropertyTreeState setter and remove meaningless DCHECK The common pattern is to accept PassRefPtr in setters because it will allow zero cost ownership transfer. Also the DCHECK for non-unique ownership is removed, because it never worked as intended. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2572423002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [SPv2] Use PassRefPtr in PropertyTreeState setter and remove meaningless DCHECK The common pattern is to accept PassRefPtr in setters because it will allow zero cost ownership transfer. Also the DCHECK for non-unique ownership is removed, because it never worked as intended. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2572423002 ========== to ========== [SPv2] Use PassRefPtr in PropertyTreeState setter and remove meaningless DCHECK The common pattern is to accept PassRefPtr in setters because it will allow zero cost ownership transfer. Also the DCHECK for non-unique ownership is removed, because it never worked as intended. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Committed: https://crrev.com/078212bbd956d7d72333d75ca3c2d2da5fff1190 Cr-Commit-Position: refs/heads/master@{#439626} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/078212bbd956d7d72333d75ca3c2d2da5fff1190 Cr-Commit-Position: refs/heads/master@{#439626} |
