Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1753853002:
4f gradient interval builder cleanup (Closed)
Description4f gradient interval builder cleanup
Untangle the builder, for hopefully improved readability.
R=reed@google.com,mtklein@google.com,herb@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1753853002
Committed: https://skia.googlesource.com/skia/+/e27456a43c96648e05be746870124d14ff479fcb
Patch Set 1 #Patch Set 2 : extra asserts #Patch Set 3 : speculative win build fix #
Total comments: 13
Patch Set 4 : review commetns #
Messages
Total messages: 21 (9 generated)
Description was changed from ========== 4f gradient interval builder cleanup Untangle the builder, for hopefully improved readability. R=reed@google.com,mtklein@google.com,herb@google.com ========== to ========== 4f gradient interval builder cleanup Untangle the builder, for hopefully improved readability. R=reed@google.com,mtklein@google.com,herb@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753853002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753853002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753853002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753853002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... File src/effects/gradients/Sk4fGradientBase.cpp (right): https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... src/effects/gradients/Sk4fGradientBase.cpp:43: void build(std::function<void(SkColor, SkColor, SkScalar, SkScalar)> I would use: template <Func> void build(Func func) { func() } You would probably have to do the same with buildImplicitPos https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... src/effects/gradients/Sk4fGradientBase.cpp:46: buildImplicitPos(func); this-> https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... src/effects/gradients/Sk4fGradientBase.cpp:51: const SkScalar last_pos = 1 - fFirstPos; lastPos https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... src/effects/gradients/Sk4fGradientBase.cpp:53: SkScalar prev_pos = fFirstPos; prevPos https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... src/effects/gradients/Sk4fGradientBase.cpp:60: const SkScalar curr_pos = (fAdvance > 0) currPos, here and other places https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... src/effects/gradients/Sk4fGradientBase.cpp:75: void buildImplicitPos(std::function<void(SkColor, SkColor, SkScalar, SkScalar)> What is the difference between buildImplicitPos and build? https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... src/effects/gradients/Sk4fGradientBase.cpp:214: const int first_index = dx_is_pos ? 0 : shader.fColorCount - 1; camelCase
https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... File src/effects/gradients/Sk4fGradientBase.cpp (right): https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... src/effects/gradients/Sk4fGradientBase.cpp:43: void build(std::function<void(SkColor, SkColor, SkScalar, SkScalar)> On 2016/03/01 21:50:37, herb_g wrote: > I would use: > template <Func> > void build(Func func) { > > func() > } > > You would probably have to do the same with buildImplicitPos Done. https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... src/effects/gradients/Sk4fGradientBase.cpp:46: buildImplicitPos(func); On 2016/03/01 21:50:37, herb_g wrote: > this-> Done. https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... src/effects/gradients/Sk4fGradientBase.cpp:51: const SkScalar last_pos = 1 - fFirstPos; On 2016/03/01 21:50:37, herb_g wrote: > lastPos Done. https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... src/effects/gradients/Sk4fGradientBase.cpp:53: SkScalar prev_pos = fFirstPos; On 2016/03/01 21:50:37, herb_g wrote: > prevPos Done. https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... src/effects/gradients/Sk4fGradientBase.cpp:60: const SkScalar curr_pos = (fAdvance > 0) On 2016/03/01 21:50:36, herb_g wrote: > currPos, here and other places Done. https://codereview.chromium.org/1753853002/diff/40001/src/effects/gradients/S... src/effects/gradients/Sk4fGradientBase.cpp:75: void buildImplicitPos(std::function<void(SkColor, SkColor, SkScalar, SkScalar)> On 2016/03/01 21:50:36, herb_g wrote: > What is the difference between buildImplicitPos and build? build() handles the case of explicit color stop positions provided by clients, while buildImplicitPos() deals with implicit positioning (stops are distributed evenly). Added comments.
The CQ bit was checked by fmalita@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753853002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753853002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by fmalita@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1753853002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1753853002/60001
Message was sent while issue was closed.
Description was changed from ========== 4f gradient interval builder cleanup Untangle the builder, for hopefully improved readability. R=reed@google.com,mtklein@google.com,herb@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== 4f gradient interval builder cleanup Untangle the builder, for hopefully improved readability. R=reed@google.com,mtklein@google.com,herb@google.com GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/e27456a43c96648e05be746870124d14ff479fcb ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/e27456a43c96648e05be746870124d14ff479fcb |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
