Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1585813004:
SkValue: SkXfermode (Closed)
|
Created:
4 years, 11 months ago by hal.canary Modified:
4 years, 11 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSkValue: SkXfermode
Implement:
template<> SkValue SkToValue<SkXfermode>(const SkXfermode*);
template<> bool SkFromValue<SkXfermode*>(const SkValue&, SkXfermode**);
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1585813004
NOTRY=true
Committed: https://skia.googlesource.com/skia/+/27a6e86fb17fce7ce962b9080eae36926e87d568
Patch Set 1 #Patch Set 2 : unit test! #Patch Set 3 : 2016-01-14 (Thursday) 16:27:56 EST #
Total comments: 6
Patch Set 4 : response, also dump #
Total comments: 33
Patch Set 5 : 2016-01-15 (Friday) 08:39:02 EST #Patch Set 6 : 2016-01-15 (Friday) 11:08:28 EST #
Total comments: 6
Patch Set 7 : 2016-01-15 (Friday) 14:43:14 EST #
Total comments: 6
Patch Set 8 : fix unit tests #Patch Set 9 : 2016-01-15 (Friday) 15:39:23 EST #Patch Set 10 : type assertions #Patch Set 11 : 2016-01-21 (Thursday) 10:45:48 EST #Patch Set 12 : wrong cast fixed #Patch Set 13 : fix macro redef #
Total comments: 10
Patch Set 14 : 2016-01-21 (Thursday) 12:06:57 EST #Patch Set 15 : comments from mtklein #
Total comments: 14
Patch Set 16 : mor changes #Patch Set 17 : get() #
Total comments: 4
Patch Set 18 : missed one #Patch Set 19 : space #
Dependent Patchsets: Messages
Total messages: 80 (34 generated)
Description was changed from ========== SkValue from SkXfermode ========== to ========== SkValue from SkXfermode GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
halcanary@google.com changed reviewers: + mtklein@google.com
PTAL
https://codereview.chromium.org/1585813004/diff/40001/include/core/SkXfermode.h File include/core/SkXfermode.h (right): https://codereview.chromium.org/1585813004/diff/40001/include/core/SkXfermode... include/core/SkXfermode.h:233: virtual SkValue represent() const = 0; -> value(), asValue() ? https://codereview.chromium.org/1585813004/diff/40001/src/core/SkValue.h File src/core/SkValue.h (left): https://codereview.chromium.org/1585813004/diff/40001/src/core/SkValue.h#oldc... src/core/SkValue.h:30: // Each Object type may define its own namespace of Key values, Move this left and over the typedef, and update to something like // Each Object type may define its own namespace of Key values. // Their namespace is distinct from other uint32_ts, like Type. ? https://codereview.chromium.org/1585813004/diff/40001/src/effects/SkArithmeti... File src/effects/SkArithmeticMode.cpp (right): https://codereview.chromium.org/1585813004/diff/40001/src/effects/SkArithmeti... src/effects/SkArithmeticMode.cpp:41: SkValue represent() const override { I don't yet see a need for key enums to be visible outside where it's implemented. enum { k0, k1, k2, k3, kEnforcePMColor }; SkValue represent() const override { auto value = SkValue::Object(SkValue::ArithmeticXfermode); value.set(k0, SkValue::fromF32(fK[0]); ... value.set(kEnforcePMColor, SkValue::FromS32(fEnforcePMColor)); return value; }
https://codereview.chromium.org/1585813004/diff/40001/include/core/SkXfermode.h File include/core/SkXfermode.h (right): https://codereview.chromium.org/1585813004/diff/40001/include/core/SkXfermode... include/core/SkXfermode.h:233: virtual SkValue represent() const = 0; On 2016/01/14 at 22:58:28, mtklein wrote: > -> value(), asValue() ? asValue. done https://codereview.chromium.org/1585813004/diff/40001/src/core/SkValue.h File src/core/SkValue.h (left): https://codereview.chromium.org/1585813004/diff/40001/src/core/SkValue.h#oldc... src/core/SkValue.h:30: // Each Object type may define its own namespace of Key values, On 2016/01/14 at 22:58:28, mtklein wrote: > Move this left and over the typedef, and update to something like > > // Each Object type may define its own namespace of Key values. > // Their namespace is distinct from other uint32_ts, like Type. > > ? done https://codereview.chromium.org/1585813004/diff/40001/src/effects/SkArithmeti... File src/effects/SkArithmeticMode.cpp (right): https://codereview.chromium.org/1585813004/diff/40001/src/effects/SkArithmeti... src/effects/SkArithmeticMode.cpp:41: SkValue represent() const override { On 2016/01/14 at 22:58:28, mtklein wrote: > I don't yet see a need for key enums to be visible outside where it's implemented. > > > enum { k0, k1, k2, k3, kEnforcePMColor }; > > SkValue represent() const override { > auto value = SkValue::Object(SkValue::ArithmeticXfermode); > value.set(k0, SkValue::fromF32(fK[0]); > ... > value.set(kEnforcePMColor, SkValue::FromS32(fEnforcePMColor)); > return value; > } done.
The CQ bit was checked by halcanary@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/1585813004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585813004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp File src/core/SkValue.cpp (right): https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:24: reinterpret_cast<SkData*>(fBytes)->ref(); WTF? If you want to have an SkData* member, just change it to be an SkData*. https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:26: fObject->ref(); This makes no sense. You can't share anything that's mutable. If you change one object you'll change the other. https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:32: new(&o) SkValue(); space after new? https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:53: } else if (fType > 0xFF) { Let's add private: bool isObject() const { fType > 0xFF } https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:60: v.fType = SkValue::S32; in this scope you can just write S32, etc. https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:96: int32_t SkValue::s32() const { return SkValue::S32 == fType ? fS32 : 0; } These should all assert they're the right type. https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:112: return fObject->fMap.size(); let's just have count() work for Bytes for now. https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:128: return map.find(k) != map.end() ? &map[k] : nullptr; This does the hash lookup twice. const auto& map = fObject->fMap; for (auto it = map.find(k); it != map.end(); ) { return &*it; } return nullptr; https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.h File src/core/SkValue.h (right): https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.h#newc... src/core/SkValue.h:73: class Obj; ? https://codereview.chromium.org/1585813004/diff/60001/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.chromium.org/1585813004/diff/60001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:767: enum { ProcCoeffXfermode_Mode }; This name does not fit any naming scheme we use in Skia. Worse, it looks like it's a member of some Mode enum with value ProcCoeffXfermode. Why are you complicating this? Just write kMode. https://codereview.chromium.org/1585813004/diff/60001/src/utils/debugger/SkDe... File src/utils/debugger/SkDebugCanvas.cpp (left): https://codereview.chromium.org/1585813004/diff/60001/src/utils/debugger/SkDe... src/utils/debugger/SkDebugCanvas.cpp:57: }; Let's not bother with this xfermode. We don't serialize it. https://codereview.chromium.org/1585813004/diff/60001/tests/ValueTest.cpp File tests/ValueTest.cpp (right): https://codereview.chromium.org/1585813004/diff/60001/tests/ValueTest.cpp#new... tests/ValueTest.cpp:30: DEF_TEST(Value_Xfermode, r) { Let's test SkValue with new types scoped to this file. As written, these tests are coupled to the implementation details of SkXfermode, but have little to do with SkXfermodes themselves. It's annoying to other people working on SkXfermodes to have to update tests like this, particularly when we can just make up our own types to test with (using the top half of Test's range). https://codereview.chromium.org/1585813004/diff/60001/tests/ValueTest.cpp#new... tests/ValueTest.cpp:73: #define PRId32 "d" Why are you making this complicated? Just write %d and %u like normal. https://codereview.chromium.org/1585813004/diff/60001/tests/ValueTest.cpp#new... tests/ValueTest.cpp:94: static void dump(const SkValue& val, SkWStream* o) { Just use SkDebugf?
https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp File src/core/SkValue.cpp (right): https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:24: reinterpret_cast<SkData*>(fBytes)->ref(); On 2016/01/15 at 00:25:22, mtklein wrote: > WTF? If you want to have an SkData* member, just change it to be an SkData*. I started by trying to implelment this without changing your header, just to test my other code. done. https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:26: fObject->ref(); On 2016/01/15 at 00:25:22, mtklein wrote: > This makes no sense. You can't share anything that's mutable. If you change one object you'll change the other. If I add a flag that gets set on the first copy that makes the entire thing immutable, will that work? https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:32: new(&o) SkValue(); On 2016/01/15 at 00:25:22, mtklein wrote: > space after new? done https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:53: } else if (fType > 0xFF) { On 2016/01/15 at 00:25:22, mtklein wrote: > Let's add > > private: > bool isObject() const { fType > 0xFF } done. https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:60: v.fType = SkValue::S32; On 2016/01/15 at 00:25:22, mtklein wrote: > in this scope you can just write S32, etc. done. https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:96: int32_t SkValue::s32() const { return SkValue::S32 == fType ? fS32 : 0; } On 2016/01/15 at 00:25:22, mtklein wrote: > These should all assert they're the right type. done. https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:112: return fObject->fMap.size(); On 2016/01/15 at 00:25:22, mtklein wrote: > let's just have count() work for Bytes for now. why? https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:128: return map.find(k) != map.end() ? &map[k] : nullptr; On 2016/01/15 at 00:25:22, mtklein wrote: > This does the hash lookup twice. > > const auto& map = fObject->fMap; > for (auto it = map.find(k); it != map.end(); ) { > return &*it; > } > return nullptr; done. reading the specs it looked like the iterator returned a copy. but testing shows that it doesn't. https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.h File src/core/SkValue.h (right): https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.h#newc... src/core/SkValue.h:73: class Obj; On 2016/01/15 at 00:25:22, mtklein wrote: > ? SkValue::Object was an enum value, too. The compiler borked when I asked for a `new SkValue::Object`. https://codereview.chromium.org/1585813004/diff/60001/src/core/SkXfermode.cpp File src/core/SkXfermode.cpp (right): https://codereview.chromium.org/1585813004/diff/60001/src/core/SkXfermode.cpp... src/core/SkXfermode.cpp:767: enum { ProcCoeffXfermode_Mode }; On 2016/01/15 at 00:25:22, mtklein wrote: > This name does not fit any naming scheme we use in Skia. Worse, it looks like it's a member of some Mode enum with value ProcCoeffXfermode. > > Why are you complicating this? Just write kMode. done https://codereview.chromium.org/1585813004/diff/60001/src/utils/debugger/SkDe... File src/utils/debugger/SkDebugCanvas.cpp (left): https://codereview.chromium.org/1585813004/diff/60001/src/utils/debugger/SkDe... src/utils/debugger/SkDebugCanvas.cpp:57: }; On 2016/01/15 at 00:25:22, mtklein wrote: > Let's not bother with this xfermode. We don't serialize it. done https://codereview.chromium.org/1585813004/diff/60001/tests/ValueTest.cpp File tests/ValueTest.cpp (right): https://codereview.chromium.org/1585813004/diff/60001/tests/ValueTest.cpp#new... tests/ValueTest.cpp:30: DEF_TEST(Value_Xfermode, r) { On 2016/01/15 at 00:25:22, mtklein wrote: > Let's test SkValue with new types scoped to this file. > > As written, these tests are coupled to the implementation details of SkXfermode, but have little to do with SkXfermodes themselves. It's annoying to other people working on SkXfermodes to have to update tests like this, particularly when we can just make up our own types to test with (using the top half of Test's range). okay. https://codereview.chromium.org/1585813004/diff/60001/tests/ValueTest.cpp#new... tests/ValueTest.cpp:73: #define PRId32 "d" On 2016/01/15 at 00:25:22, mtklein wrote: > Why are you making this complicated? Just write %d and %u like normal. that assumes that ints are 32 bits. https://codereview.chromium.org/1585813004/diff/60001/tests/ValueTest.cpp#new... tests/ValueTest.cpp:94: static void dump(const SkValue& val, SkWStream* o) { On 2016/01/15 at 00:25:22, mtklein wrote: > Just use SkDebugf? Ease of reuse. I may eventually reuse this code as a DM sink.
https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp File src/core/SkValue.cpp (right): https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:26: fObject->ref(); On 2016/01/15 13:38:56, Hal Canary wrote: > On 2016/01/15 at 00:25:22, mtklein wrote: > > This makes no sense. You can't share anything that's mutable. If you change > one object you'll change the other. > > If I add a flag that gets set on the first copy that makes the entire thing > immutable, will that work? Yes, but that's an extremely fragile design. If you want to do that, pass ownership of the map from a MutableObject to a ConstObject... change its type. Then you'll need to copy (really copy) it back to a MutableObject on the next write, etc. It also all needs to be thread safe. So don't do any of that. Copy. https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:112: return fObject->fMap.size(); On 2016/01/15 13:38:56, Hal Canary wrote: > On 2016/01/15 at 00:25:22, mtklein wrote: > > let's just have count() work for Bytes for now. > > why? We have foreach(). https://codereview.chromium.org/1585813004/diff/60001/src/core/SkValue.cpp#ne... src/core/SkValue.cpp:128: return map.find(k) != map.end() ? &map[k] : nullptr; On 2016/01/15 13:38:56, Hal Canary wrote: > On 2016/01/15 at 00:25:22, mtklein wrote: > > This does the hash lookup twice. > > > > const auto& map = fObject->fMap; > > for (auto it = map.find(k); it != map.end(); ) { > > return &*it; > > } > > return nullptr; > > done. reading the specs it looked like the iterator returned a copy. but > testing shows that it doesn't. That map iterator is a little surprising, I admit, especially on a team that doesn't have experience using the STL.
You're making this pointlessly complicated. Stop. https://codereview.chromium.org/1585813004/diff/60001/tests/ValueTest.cpp File tests/ValueTest.cpp (right): https://codereview.chromium.org/1585813004/diff/60001/tests/ValueTest.cpp#new... tests/ValueTest.cpp:73: #define PRId32 "d" On 2016/01/15 13:38:56, Hal Canary wrote: > On 2016/01/15 at 00:25:22, mtklein wrote: > > Why are you making this complicated? Just write %d and %u like normal. > > that assumes that ints are 32 bits. ints are 32 bits. https://codereview.chromium.org/1585813004/diff/60001/tests/ValueTest.cpp#new... tests/ValueTest.cpp:94: static void dump(const SkValue& val, SkWStream* o) { On 2016/01/15 13:38:56, Hal Canary wrote: > On 2016/01/15 at 00:25:22, mtklein wrote: > > Just use SkDebugf? > > Ease of reuse. I may eventually reuse this code as a DM sink. I don't care unless you actually do it. Don't write complicated code until it needs to exist.
Okay! Everything is slightly simpler now. Tests are better too. (I test nested objects). On 2016/01/15 at 14:16:09, mtklein wrote: > You're making this pointlessly complicated. Stop. > > https://codereview.chromium.org/1585813004/diff/60001/tests/ValueTest.cpp#new... > tests/ValueTest.cpp:94: static void dump(const SkValue& val, SkWStream* o) { > On 2016/01/15 13:38:56, Hal Canary wrote: > > On 2016/01/15 at 00:25:22, mtklein wrote: > > > Just use SkDebugf? > > > > Ease of reuse. I may eventually reuse this code as a DM sink. > > I don't care unless you actually do it. Don't write complicated code until it needs to exist. Ah, but I didn't have to write it; I just cut-and-pasted from another CL.
> Ah, but I didn't have to write it; I just cut-and-pasted from another CL. That is incredibly, vastly worse. I'm getting tired of fighting you on these things. If you want to keep fighting silly battles you should work on something else.
https://codereview.chromium.org/1585813004/diff/100001/src/core/SkValue.cpp File src/core/SkValue.cpp (right): https://codereview.chromium.org/1585813004/diff/100001/src/core/SkValue.cpp#n... src/core/SkValue.cpp:14: // TODO(mtklein): replace with something less fragile later. Goodness no. Copy. Not going to ask again. https://codereview.chromium.org/1585813004/diff/100001/src/core/SkValue.h File src/core/SkValue.h (right): https://codereview.chromium.org/1585813004/diff/100001/src/core/SkValue.h#new... src/core/SkValue.h:91: bool isObject() const { return fType > 0xFF; } kMaxBuiltin? https://codereview.chromium.org/1585813004/diff/100001/src/utils/debugger/SkD... File src/utils/debugger/SkDebugCanvas.cpp (right): https://codereview.chromium.org/1585813004/diff/100001/src/utils/debugger/SkD... src/utils/debugger/SkDebugCanvas.cpp:59: SkValue asValue() const override { return SkValue(); } Let's make this the default impl on SkXfermode, at least for now? That makes working with values opt-in rather than opt-out as we've got now.
On 2016/01/15 at 17:22:16, mtklein wrote: > That is incredibly, vastly worse. > > I'm getting tired of fighting you on these things. If you want to keep fighting silly battles you should work on something else. I have removed all of the offending code, but am keeping it around locally so I can test as I go.
https://codereview.chromium.org/1585813004/diff/100001/src/core/SkValue.cpp File src/core/SkValue.cpp (right): https://codereview.chromium.org/1585813004/diff/100001/src/core/SkValue.cpp#n... src/core/SkValue.cpp:14: // TODO(mtklein): replace with something less fragile later. On 2016/01/15 at 17:26:20, mtklein wrote: > Goodness no. Copy. Not going to ask again. done https://codereview.chromium.org/1585813004/diff/100001/src/core/SkValue.h File src/core/SkValue.h (right): https://codereview.chromium.org/1585813004/diff/100001/src/core/SkValue.h#new... src/core/SkValue.h:91: bool isObject() const { return fType > 0xFF; } On 2016/01/15 at 17:26:20, mtklein wrote: > kMaxBuiltin? done https://codereview.chromium.org/1585813004/diff/100001/src/utils/debugger/SkD... File src/utils/debugger/SkDebugCanvas.cpp (right): https://codereview.chromium.org/1585813004/diff/100001/src/utils/debugger/SkD... src/utils/debugger/SkDebugCanvas.cpp:59: SkValue asValue() const override { return SkValue(); } On 2016/01/15 at 17:26:20, mtklein wrote: > Let's make this the default impl on SkXfermode, at least for now? That makes working with values opt-in rather than opt-out as we've got now. done. I'll do this with all of the SkFooPaintEffect classes.
https://codereview.chromium.org/1585813004/diff/120001/src/core/SkValue.cpp File src/core/SkValue.cpp (right): https://codereview.chromium.org/1585813004/diff/120001/src/core/SkValue.cpp#n... src/core/SkValue.cpp:95: return Bytes == fType ? fBytes : nullptr; Assert type? https://codereview.chromium.org/1585813004/diff/120001/src/core/SkValue.cpp#n... src/core/SkValue.cpp:99: if (this->isObject()) { Assert type? https://codereview.chromium.org/1585813004/diff/120001/src/core/SkValue.cpp#n... src/core/SkValue.cpp:104: const SkValue* SkValue::get(SkValue::Key k) const { Assert type? https://codereview.chromium.org/1585813004/diff/120001/src/core/SkValue.cpp#n... src/core/SkValue.cpp:112: void SkValue::foreach(std::function<void(Key, const SkValue&)> fn) const { Assert type? https://codereview.chromium.org/1585813004/diff/120001/src/core/SkValue.h File src/core/SkValue.h (right): https://codereview.chromium.org/1585813004/diff/120001/src/core/SkValue.h#new... src/core/SkValue.h:16: #define SK_VALUE_OBJECT_TYPES(M) \ Let's expand this in place (for now). (For now) It's only used in that one place, so it's just noise (for now). https://codereview.chromium.org/1585813004/diff/120001/tests/ValueTest.cpp File tests/ValueTest.cpp (right): https://codereview.chromium.org/1585813004/diff/120001/tests/ValueTest.cpp#ne... tests/ValueTest.cpp:15: #define EXAMPLES(FN) \ Let's expand this out?
The CQ bit was checked by halcanary@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/1585813004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585813004/180001
Description was changed from ========== SkValue from SkXfermode GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkValue: Implementation and SkValue from SkXfermode\ Also, unit test for implementation. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
lgtm
Description was changed from ========== SkValue: Implementation and SkValue from SkXfermode\ Also, unit test for implementation. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkValue: Implementation and SkValue from SkXfermode Also, unit test for implementation. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
halcanary@google.com changed reviewers: + reed@google.com
reed@: API
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== SkValue: Implementation and SkValue from SkXfermode Also, unit test for implementation. GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkValue: SkXfermode Implement: template<> SkValue SkToValue<SkXfermode>(const SkXfermode*); template<> bool SkFromValue<SkXfermode*>(const SkValue&, SkXfermode**); GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by halcanary@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/1585813004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585813004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
mtklein@, this CL has now been rebased on all of the other SkValue commits, and is much cleaner for it. Please re-review it, specifically the SkFromValue<SkXfermode> code. reed@, SkXfermode::asValue() is now marked private.
The CQ bit was checked by halcanary@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/1585813004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585813004/220001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
The CQ bit was checked by halcanary@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/1585813004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585813004/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)
The CQ bit was checked by halcanary@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/1585813004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585813004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkArithmet... File src/effects/SkArithmeticMode.cpp (right): https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkArithmet... src/effects/SkArithmeticMode.cpp:58: SkValue asValue() const override { Boy that's super verbose. These seem like a good place for a very localized using: using namespace SkValueKeys::ArithmeticXfermode; auto value = SkValue::Object(SkValue::ArithmeticXfermode); value.set(kK0, SkValue::FromF32(fK[0])); ... https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkPixelXor... File src/effects/SkPixelXorXfermode.cpp (right): https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkPixelXor... src/effects/SkPixelXorXfermode.cpp:40: enum { kOpColor }; ? https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkToFromVa... File src/effects/SkToFromValue.cpp (right): https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:20: #define OPTIONAL_KEY(VAL, KEY, PTR) \ Update the spacing now that you've added _KEY? https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:87: switch (val.type()) { I think we'll be happier in the long term if we split apart the type switch and hide the logic of how to parse each type in a little static function (where we can using namespace the appropriate keys). switch (val.type()) { case SkValue:: DefaultXfermode: return from_DefaultXfermode(val, dst); case SkValue::ArithmeticXfermode: return from_ArithmeticXfermode(val, dst); ... default: REQUIRE(false); } https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:92: float k[4] = {0.0f, 0.0f, 0.0f, 0.0f}; There's no need to initialize mandatory things, right?
Might want to spin off the include/ changes to Mike can LGTM them and we waddle on without him?
https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkArithmet... File src/effects/SkArithmeticMode.cpp (right): https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkArithmet... src/effects/SkArithmeticMode.cpp:58: SkValue asValue() const override { On 2016/01/21 at 17:46:18, mtklein wrote: > Boy that's super verbose. > > These seem like a good place for a very localized using: > using namespace SkValueKeys::ArithmeticXfermode; > auto value = SkValue::Object(SkValue::ArithmeticXfermode); > value.set(kK0, SkValue::FromF32(fK[0])); > ... done. https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkPixelXor... File src/effects/SkPixelXorXfermode.cpp (right): https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkPixelXor... src/effects/SkPixelXorXfermode.cpp:40: enum { kOpColor }; On 2016/01/21 at 17:46:19, mtklein wrote: > ? forgot that one. good catch https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkToFromVa... File src/effects/SkToFromValue.cpp (right): https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:20: #define OPTIONAL_KEY(VAL, KEY, PTR) \ On 2016/01/21 at 17:46:19, mtklein wrote: > Update the spacing now that you've added _KEY? done. https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:87: switch (val.type()) { On 2016/01/21 at 17:46:19, mtklein wrote: > I think we'll be happier in the long term if we split apart the type switch and hide the logic of how to parse each type in a little static function (where we can using namespace the appropriate keys). > > switch (val.type()) { > case SkValue:: DefaultXfermode: return from_DefaultXfermode(val, dst); > case SkValue::ArithmeticXfermode: return from_ArithmeticXfermode(val, dst); > ... > default: REQUIRE(false); > } I was considering that, but never got around to it. Done. https://codereview.chromium.org/1585813004/diff/240001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:92: float k[4] = {0.0f, 0.0f, 0.0f, 0.0f}; On 2016/01/21 at 17:46:19, mtklein wrote: > There's no need to initialize mandatory things, right? right. that was leftover.
private api lgtm
https://codereview.chromium.org/1585813004/diff/280001/src/core/SkValueKeys.h File src/core/SkValueKeys.h (right): https://codereview.chromium.org/1585813004/diff/280001/src/core/SkValueKeys.h... src/core/SkValueKeys.h:11: #include "SkValue.h" Not really needed? https://codereview.chromium.org/1585813004/diff/280001/src/core/SkValueKeys.h... src/core/SkValueKeys.h:14: namespace ArithmeticXfermode { enum { kK0, kK1, kK2, kK3, kEnforcePMColor }; } Might want to indent these inner namespaces a bit? https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... File src/effects/SkToFromValue.cpp (right): https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:20: #define OPTIONAL_KEY(VAL, KEY, PTR) \ How far do you think we can get without having more macros than REQUIRE? template <typename T> bool getT(const SkValue& obj, SkValue::Key key, T* ptr) { auto v = obj.get(key); return v ? SkFromValue(*v, ptr) : false; } I think that makes the equivalent of OPTIONAL_KEY just int32_t i = 0; getT(obj, key, &i); and the equivalent of MANDATORY_KEY just int32_t i; REQUIRE(getT(obj, key, &i)); Seem right? https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:78: static bool defaultxfermode_from_value(const SkValue& val, I'd slightly prefer from_value_DefaultXfermode, etc, so these look static but also look exactly like the Type / type they're working with. https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:127: return defaultxfermode_from_value(val, dst); This would be easier to read if they were each one line. Does it run over 100 columns? If so, can we shorten names so it doesn't?
https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... File src/effects/SkToFromValue.cpp (right): https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:87: int32_t enforce = true; One of my goals in reducing the number of macros we have is to make us feel good about writing things like this field naturally the way we want them expressed, rather than how the macros make us do it. E.g. bool enforcePMColor = true; if (auto v = val.get(kEnforcePMColor)) { enforcePMColor = SkToBool(v->s32()); } some more fewer-macro ideas below: https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:109: uint32_t opColor = 0; Same deal here: uint32_t color; REQUIRE(getT(val, kOpColor, &color)); dst->reset(SkPixelXorXfermode::Create(color)); return true; OR auto color = val.get(kOpColor); REQUIRE(color); dst->reset(SkPixelXorXfermode::Create(color->u32())); return true; https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:117: uint32_t mode = 0; Same deal here: auto mode = val.get(kMode); REQUIRE(mode); dst->reset(SkXfermode::Create((SkXfermode::Mode)mode->u32())); return true;
https://codereview.chromium.org/1585813004/diff/280001/src/core/SkValueKeys.h File src/core/SkValueKeys.h (right): https://codereview.chromium.org/1585813004/diff/280001/src/core/SkValueKeys.h... src/core/SkValueKeys.h:11: #include "SkValue.h" On 2016/01/21 at 19:01:23, mtklein wrote: > Not really needed? done https://codereview.chromium.org/1585813004/diff/280001/src/core/SkValueKeys.h... src/core/SkValueKeys.h:14: namespace ArithmeticXfermode { enum { kK0, kK1, kK2, kK3, kEnforcePMColor }; } On 2016/01/21 at 19:01:23, mtklein wrote: > Might want to indent these inner namespaces a bit? done https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... File src/effects/SkToFromValue.cpp (right): https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:20: #define OPTIONAL_KEY(VAL, KEY, PTR) \ On 2016/01/21 at 19:01:23, mtklein wrote: > How far do you think we can get without having more macros than REQUIRE? > > template <typename T> > bool getT(const SkValue& obj, SkValue::Key key, T* ptr) { > auto v = obj.get(key); > return v ? SkFromValue(*v, ptr) : false; > } > > I think that makes the equivalent of OPTIONAL_KEY just > int32_t i = 0; > getT(obj, key, &i); > and the equivalent of MANDATORY_KEY just > int32_t i; > REQUIRE(getT(obj, key, &i)); > > Seem right? almost. OPTIONAL_KEY should still return false if the key type is a mismatch. Is what I changed it to better or worse? https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:78: static bool defaultxfermode_from_value(const SkValue& val, On 2016/01/21 at 19:01:23, mtklein wrote: > I'd slightly prefer from_value_DefaultXfermode, etc, so these look static but also look exactly like the Type / type they're working with. done. https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:127: return defaultxfermode_from_value(val, dst); On 2016/01/21 at 19:01:23, mtklein wrote: > This would be easier to read if they were each one line. Does it run over 100 columns? If so, can we shorten names so it doesn't? done.
https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... File src/effects/SkToFromValue.cpp (right): https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:20: #define OPTIONAL_KEY(VAL, KEY, PTR) \ On 2016/01/21 19:33:40, Hal Canary wrote: > On 2016/01/21 at 19:01:23, mtklein wrote: > > How far do you think we can get without having more macros than REQUIRE? > > > > template <typename T> > > bool getT(const SkValue& obj, SkValue::Key key, T* ptr) { > > auto v = obj.get(key); > > return v ? SkFromValue(*v, ptr) : false; > > } > > > > I think that makes the equivalent of OPTIONAL_KEY just > > int32_t i = 0; > > getT(obj, key, &i); > > and the equivalent of MANDATORY_KEY just > > int32_t i; > > REQUIRE(getT(obj, key, &i)); > > > > Seem right? > > almost. OPTIONAL_KEY should still return false if the key type is a mismatch. Why? We have a default value already, so it's not super-fatal if we find the wrong type there. int32_t i = 0; getT(obj, key, &i); would assert in Debug mode (catches our bugs) and carry on with i = 0 without error in Release mode (safe) if the key doesn't point to an S32.
On 2016/01/21 at 20:01:33, mtklein wrote: > https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... > File src/effects/SkToFromValue.cpp (right): > > https://codereview.chromium.org/1585813004/diff/280001/src/effects/SkToFromVa... > src/effects/SkToFromValue.cpp:20: #define OPTIONAL_KEY(VAL, KEY, PTR) \ > On 2016/01/21 19:33:40, Hal Canary wrote: > > On 2016/01/21 at 19:01:23, mtklein wrote: > > > How far do you think we can get without having more macros than REQUIRE? > > > > > > template <typename T> > > > bool getT(const SkValue& obj, SkValue::Key key, T* ptr) { > > > auto v = obj.get(key); > > > return v ? SkFromValue(*v, ptr) : false; > > > } > > > > > > I think that makes the equivalent of OPTIONAL_KEY just > > > int32_t i = 0; > > > getT(obj, key, &i); > > > and the equivalent of MANDATORY_KEY just > > > int32_t i; > > > REQUIRE(getT(obj, key, &i)); > > > > > > Seem right? > > > > almost. OPTIONAL_KEY should still return false if the key type is a mismatch. > > Why? We have a default value already, so it's not super-fatal if we find the wrong type there. > int32_t i = 0; > getT(obj, key, &i); > would assert in Debug mode (catches our bugs) and carry on with i = 0 without error in Release mode (safe) if the key doesn't point to an S32. excellent point. Done.
The CQ bit was checked by halcanary@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/1585813004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585813004/340001
lgtm, with some small nits https://codereview.chromium.org/1585813004/diff/320001/src/effects/SkToFromVa... File src/effects/SkToFromValue.cpp (right): https://codereview.chromium.org/1585813004/diff/320001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:56: template<> bool SkFromValue<SkMatrix>(const SkValue& val, SkMatrix* m){ space between ) and { ? https://codereview.chromium.org/1585813004/diff/320001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:121: case SkValue::LerpXfermode: return lerpxfermode_from_value(val, dst); poor little lerpxfermode doesn't get to play with the Big Boys
https://codereview.chromium.org/1585813004/diff/320001/src/effects/SkToFromVa... File src/effects/SkToFromValue.cpp (right): https://codereview.chromium.org/1585813004/diff/320001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:56: template<> bool SkFromValue<SkMatrix>(const SkValue& val, SkMatrix* m){ On 2016/01/21 at 20:25:11, mtklein wrote: > space between ) and { ? done https://codereview.chromium.org/1585813004/diff/320001/src/effects/SkToFromVa... src/effects/SkToFromValue.cpp:121: case SkValue::LerpXfermode: return lerpxfermode_from_value(val, dst); On 2016/01/21 at 20:25:11, mtklein wrote: > poor little lerpxfermode doesn't get to play with the Big Boys I already caught that a minute ago.
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com, mtklein@google.com Link to the patchset: https://codereview.chromium.org/1585813004/#ps360001 (title: "space")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1585813004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585813004/360001
Description was changed from ========== SkValue: SkXfermode Implement: template<> SkValue SkToValue<SkXfermode>(const SkXfermode*); template<> bool SkFromValue<SkXfermode*>(const SkValue&, SkXfermode**); GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== SkValue: SkXfermode Implement: template<> SkValue SkToValue<SkXfermode>(const SkXfermode*); template<> bool SkFromValue<SkXfermode*>(const SkValue&, SkXfermode**); GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXCLUDE_TRYBOTS=tryserver.skia: ==========
Description was changed from ========== SkValue: SkXfermode Implement: template<> SkValue SkToValue<SkXfermode>(const SkXfermode*); template<> bool SkFromValue<SkXfermode*>(const SkValue&, SkXfermode**); GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXCLUDE_TRYBOTS=tryserver.skia: ========== to ========== SkValue: SkXfermode Implement: template<> SkValue SkToValue<SkXfermode>(const SkXfermode*); template<> bool SkFromValue<SkXfermode*>(const SkValue&, SkXfermode**); GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXCLUDE_TRYBOTS=tryserver.skia:Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot ==========
The CQ bit was unchecked by halcanary@google.com
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1585813004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585813004/360001
Description was changed from ========== SkValue: SkXfermode Implement: template<> SkValue SkToValue<SkXfermode>(const SkXfermode*); template<> bool SkFromValue<SkXfermode*>(const SkValue&, SkXfermode**); GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXCLUDE_TRYBOTS=tryserver.skia:Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot ========== to ========== SkValue: SkXfermode Implement: template<> SkValue SkToValue<SkXfermode>(const SkXfermode*); template<> bool SkFromValue<SkXfermode*>(const SkValue&, SkXfermode**); GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXCLUDE_TRYBOTS=client.skia:Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot ==========
The CQ bit was unchecked by halcanary@google.com
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1585813004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585813004/360001
The CQ bit was unchecked by halcanary@google.com
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1585813004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585813004/360001
The CQ bit was unchecked by halcanary@google.com
Description was changed from ========== SkValue: SkXfermode Implement: template<> SkValue SkToValue<SkXfermode>(const SkXfermode*); template<> bool SkFromValue<SkXfermode*>(const SkValue&, SkXfermode**); GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... CQ_EXCLUDE_TRYBOTS=client.skia:Test-Ubuntu-GCC-ShuttleA-GPU-GTX660-x86_64-Release-Trybot ========== to ========== SkValue: SkXfermode Implement: template<> SkValue SkToValue<SkXfermode>(const SkXfermode*); template<> bool SkFromValue<SkXfermode*>(const SkValue&, SkXfermode**); GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... NOTRY=true ==========
The CQ bit was checked by halcanary@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1585813004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1585813004/360001
Message was sent while issue was closed.
Description was changed from ========== SkValue: SkXfermode Implement: template<> SkValue SkToValue<SkXfermode>(const SkXfermode*); template<> bool SkFromValue<SkXfermode*>(const SkValue&, SkXfermode**); GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... NOTRY=true ========== to ========== SkValue: SkXfermode Implement: template<> SkValue SkToValue<SkXfermode>(const SkXfermode*); template<> bool SkFromValue<SkXfermode*>(const SkValue&, SkXfermode**); GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... NOTRY=true Committed: https://skia.googlesource.com/skia/+/27a6e86fb17fce7ce962b9080eae36926e87d568 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001) as https://skia.googlesource.com/skia/+/27a6e86fb17fce7ce962b9080eae36926e87d568 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
