Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1925433002:
Replace SkTLazy with base::Optional (Closed)
DescriptionReplace SkTLazy with base::Optional
BUG=604860
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/ab29ff6dc90b7b652261f14617e31382f70c31b5
Cr-Commit-Position: refs/heads/master@{#390364}
Patch Set 1 #Patch Set 2 : Update Skia Patch #Patch Set 3 : Remove Patch #
Total comments: 3
Patch Set 4 : Format #Patch Set 5 : Include #
Total comments: 5
Patch Set 6 : New Comments #Patch Set 7 : No Optional #
Total comments: 2
Patch Set 8 : bungeman #
Total comments: 1
Messages
Total messages: 50 (21 generated)
Description was changed from ========== Replace SkTLazy with base::Optional BUG= COMMIT=false ========== to ========== Replace SkTLazy with base::Optional BUG= COMMIT=false CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
The CQ bit was checked by liyuqian@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1925433002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925433002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)
The CQ bit was checked by liyuqian@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1925433002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925433002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by liyuqian@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1925433002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925433002/40001
Description was changed from ========== Replace SkTLazy with base::Optional BUG= COMMIT=false CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Replace SkTLazy with base::Optional BUG= COMMIT=false CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
liyuqian@google.com changed reviewers: + bungeman@gmail.com, danakj@chromium.org, djsollen@google.com
I replaced all SkTLazy usages in Chromium with base::Optional. In Patch Set 2, it is tested against our newest Skia patch where SkTLazy is moved to include/private.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Trybots finished for the newest patch set. Please let me know if everything looks good :)
vmpstr@chromium.org changed reviewers: + vmpstr@chromium.org
https://codereview.chromium.org/1925433002/diff/40001/cc/playback/image_hijac... File cc/playback/image_hijack_canvas.cc (right): https://codereview.chromium.org/1925433002/diff/40001/cc/playback/image_hijac... cc/playback/image_hijack_canvas.cc:37: (decoded_paint_ = *paint) Can you separate this into two separate steps, please? https://codereview.chromium.org/1925433002/diff/40001/cc/playback/image_hijac... cc/playback/image_hijack_canvas.cc:48: return decoded_paint_ ? &(*decoded_paint_) : NULL; nullptr
bungeman@chromium.org changed reviewers: + bungeman@chromium.org - bungeman@gmail.com
https://codereview.chromium.org/1925433002/diff/40001/cc/playback/image_hijac... File cc/playback/image_hijack_canvas.cc (right): https://codereview.chromium.org/1925433002/diff/40001/cc/playback/image_hijac... cc/playback/image_hijack_canvas.cc:5: #include "base/optional.h" This include should go with the second set of includes. https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
I've pointed the CL at bug #604860
Description was changed from ========== Replace SkTLazy with base::Optional BUG= COMMIT=false CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Replace SkTLazy with base::Optional BUG=604860 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Thank you for the comments! I've fixed them.
The CQ bit was checked by liyuqian@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1925433002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925433002/80001
cc lgtm
https://codereview.chromium.org/1925433002/diff/80001/cc/playback/image_hijac... File cc/playback/image_hijack_canvas.cc (right): https://codereview.chromium.org/1925433002/diff/80001/cc/playback/image_hijac... cc/playback/image_hijack_canvas.cc:48: return decoded_paint_ ? &(*decoded_paint_) : nullptr; nit: no ()s https://codereview.chromium.org/1925433002/diff/80001/skia/ext/benchmarking_c... File skia/ext/benchmarking_canvas.cc (right): https://codereview.chromium.org/1925433002/diff/80001/skia/ext/benchmarking_c... skia/ext/benchmarking_canvas.cc:482: paint_ = &(*filtered_paint_); nit: no () not seeing the point of the paint_ member. seems like filtered_paint_ is enough. can you remove the paint_ member? Why is it using optional here at all? Why not just an SkPaint member, and we override it with |*paint| if one is given? Do we really care so much about not initializing the SkPaint if we'll use |*paint|?
https://codereview.chromium.org/1925433002/diff/80001/skia/ext/opacity_filter... File skia/ext/opacity_filter_canvas.cc (right): https://codereview.chromium.org/1925433002/diff/80001/skia/ext/opacity_filter... skia/ext/opacity_filter_canvas.cc:17: bool OpacityFilterCanvas::onFilter(SkTCopyOnFirstWrite<SkPaint>* paint, Type) const { Unfortunately (and I hadn't looked at this before) SkTLazy.h also provides this SkTCopyOnFirstWrite class. We need to get Chromium off of that as well in order to hide SkTLazy.h.
https://codereview.chromium.org/1925433002/diff/80001/skia/ext/opacity_filter... File skia/ext/opacity_filter_canvas.cc (right): https://codereview.chromium.org/1925433002/diff/80001/skia/ext/opacity_filter... skia/ext/opacity_filter_canvas.cc:17: bool OpacityFilterCanvas::onFilter(SkTCopyOnFirstWrite<SkPaint>* paint, Type) const { On 2016/04/27 18:32:54, bungeman (chromium) wrote: > Unfortunately (and I hadn't looked at this before) SkTLazy.h also provides this > SkTCopyOnFirstWrite class. We need to get Chromium off of that as well in order > to hide SkTLazy.h. I think that it's ok to include SkTLazy.h (especially in something like skia/ext). We just want to avoid using SkTLazy type, since base::Optional is preferred.
https://codereview.chromium.org/1925433002/diff/80001/skia/ext/opacity_filter... File skia/ext/opacity_filter_canvas.cc (right): https://codereview.chromium.org/1925433002/diff/80001/skia/ext/opacity_filter... skia/ext/opacity_filter_canvas.cc:17: bool OpacityFilterCanvas::onFilter(SkTCopyOnFirstWrite<SkPaint>* paint, Type) const { On 2016/04/27 18:36:52, vmpstr wrote: > On 2016/04/27 18:32:54, bungeman (chromium) wrote: > > Unfortunately (and I hadn't looked at this before) SkTLazy.h also provides > this > > SkTCopyOnFirstWrite class. We need to get Chromium off of that as well in > order > > to hide SkTLazy.h. > > I think that it's ok to include SkTLazy.h (especially in something like > skia/ext). We just want to avoid using SkTLazy type, since base::Optional is > preferred. That being said, opacity_filter_canvas.h includes "third_party/skia/include/utils/SkPaintFilterCanvas.h", which includes SkTLazy, so wouldn't you get it from there?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I've revised the CL according to the new comments: 1. Optional is removed from benchmarking_canvas.cc 2. opacity_filter_canvas.cc is restored because of SkTCopyOnFirstWrite. We will drop SkTCopyOnFirstWrite in later CL.
https://codereview.chromium.org/1925433002/diff/120001/cc/playback/image_hija... File cc/playback/image_hijack_canvas.cc (right): https://codereview.chromium.org/1925433002/diff/120001/cc/playback/image_hija... cc/playback/image_hijack_canvas.cc:48: return decoded_paint_ ? &(*decoded_paint_) : nullptr; danakj wanted this to just look like &*decoded_paint_, this could also be &decoded_paint_.value() since for base::Optional that's the same operation. I find this second form a bit clearer, since it makes it obvious that one is taking the address of the value and not the decoded_paint_ field itself. https://codereview.chromium.org/1925433002/diff/120001/skia/ext/benchmarking_... File skia/ext/benchmarking_canvas.cc (right): https://codereview.chromium.org/1925433002/diff/120001/skia/ext/benchmarking_... skia/ext/benchmarking_canvas.cc:473: filtered_paint_ = SkPaint(); I don't think we need this else clause, the filtered_paint_ should already be default initialized.
More fixes thanks to Ben.
lgtm
bungeman@google.com changed reviewers: + bungeman@google.com
OWNER lgtm
The CQ bit was checked by bungeman@chromium.org to run a CQ dry run
LGTM https://codereview.chromium.org/1925433002/diff/140001/cc/playback/image_hija... File cc/playback/image_hijack_canvas.cc (right): https://codereview.chromium.org/1925433002/diff/140001/cc/playback/image_hija... cc/playback/image_hijack_canvas.cc:48: return decoded_paint_ ? &decoded_paint_.value() : nullptr; yep this is cool too
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1925433002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925433002/140001
The CQ bit was unchecked by bungeman@chromium.org
The CQ bit was checked by liyuqian@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org Link to the patchset: https://codereview.chromium.org/1925433002/#ps140001 (title: "bungeman")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1925433002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1925433002/140001
Message was sent while issue was closed.
Description was changed from ========== Replace SkTLazy with base::Optional BUG=604860 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Replace SkTLazy with base::Optional BUG=604860 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/ab29ff6dc90b7b652261f14617e31382f70c31b5 Cr-Commit-Position: refs/heads/master@{#390364}
Message was sent while issue was closed.
Description was changed from ========== Replace SkTLazy with base::Optional BUG=604860 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Replace SkTLazy with base::Optional BUG=604860 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/ab29ff6dc90b7b652261f14617e31382f70c31b5 Cr-Commit-Position: refs/heads/master@{#390364} ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
