Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1362253002:
Simplify SkLayerDrawLooper
DescriptionSimplify SkLayerDrawLooper
DO NOT SUBMIT; PROOF OF CONCEPT / WIP
Drop support for adding layers to the bottom, and switch from a linked
list to a flat vector of Recs.
Patch Set 1 #
Total comments: 4
Depends on Patchset: Messages
Total messages: 18 (3 generated)
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/1362253002/diff/1/include/effects/SkLayerDraw... File include/effects/SkLayerDrawLooper.h (right): https://codereview.chromium.org/1362253002/diff/1/include/effects/SkLayerDraw... include/effects/SkLayerDrawLooper.h:15: #include <vector> We don't use std::vector in Skia (yet?) but SkTArray<T> should be very similar. We've also got SkTDArray<T> (D for Data, or Dumb) for POD T that can be a bit faster, but SkTArray<T> is usually just fine and always safe. https://codereview.chromium.org/1362253002/diff/1/include/effects/SkLayerDraw... include/effects/SkLayerDrawLooper.h:130: SkPaint* addLayerOnTop(const LayerInfo&); Can we keep the API oriented top to bottom while keeping the simplified implementation? Seems like we can still append to an array just as you're doing here, but just reverse the iteration order?
https://codereview.chromium.org/1362253002/diff/1/include/effects/SkLayerDraw... File include/effects/SkLayerDrawLooper.h (right): https://codereview.chromium.org/1362253002/diff/1/include/effects/SkLayerDraw... include/effects/SkLayerDrawLooper.h:15: #include <vector> On 2015/09/25 11:47:21, mtklein wrote: > We don't use std::vector in Skia (yet?) but SkTArray<T> should be very similar. Hm, right. Somehow I thought I saw STL usage elsewhere in Skia, but looking again it seems like only a few very specialized uses. Will update to use a Skia vector alternative. https://codereview.chromium.org/1362253002/diff/1/include/effects/SkLayerDraw... include/effects/SkLayerDrawLooper.h:130: SkPaint* addLayerOnTop(const LayerInfo&); On 2015/09/25 11:47:21, mtklein wrote: > Can we keep the API oriented top to bottom while keeping the simplified > implementation? Seems like we can still append to an array just as you're doing > here, but just reverse the iteration order? Yeah that's possible too if you think that's better. FWIW, the reasons I favored addLayerOnTop are: - I only found one use of addLayer in Chromium versus several uses of addLayerOnTop in Blink, so converting everyone to addLayerOnTop seemed slightly easier. - The name is a bit more explicit about how the layer is positioned relative to other layers (although addLayer's documentation mentions top to bottom). - A sequence of calls to addLayerOnTop will match up in the same ordering as the sequence of paints onto the canvas later, which seems easier / more natural for developers to reason about. I'm happy to look into keeping addLayer instead though. What do you think?
On 2015/09/25 16:40:17, mdempsky wrote: > https://codereview.chromium.org/1362253002/diff/1/include/effects/SkLayerDraw... > File include/effects/SkLayerDrawLooper.h (right): > > https://codereview.chromium.org/1362253002/diff/1/include/effects/SkLayerDraw... > include/effects/SkLayerDrawLooper.h:15: #include <vector> > On 2015/09/25 11:47:21, mtklein wrote: > > We don't use std::vector in Skia (yet?) but SkTArray<T> should be very > similar. > > Hm, right. Somehow I thought I saw STL usage elsewhere in Skia, but looking > again it seems like only a few very specialized uses. > > Will update to use a Skia vector alternative. Skia makes pretty heavy use of the C standard library, and small use of the C++ standard library, but none of that use really overlaps with STL containers or algorithms. This is mostly historical, but we've come to appreciate the uniform behavior of our library code across platforms. > > https://codereview.chromium.org/1362253002/diff/1/include/effects/SkLayerDraw... > include/effects/SkLayerDrawLooper.h:130: SkPaint* addLayerOnTop(const > LayerInfo&); > On 2015/09/25 11:47:21, mtklein wrote: > > Can we keep the API oriented top to bottom while keeping the simplified > > implementation? Seems like we can still append to an array just as you're > doing > > here, but just reverse the iteration order? > > Yeah that's possible too if you think that's better. FWIW, the reasons I > favored addLayerOnTop are: > > - I only found one use of addLayer in Chromium versus several uses of > addLayerOnTop in Blink, so converting everyone to addLayerOnTop seemed slightly > easier. > > - The name is a bit more explicit about how the layer is positioned relative to > other layers (although addLayer's documentation mentions top to bottom). > > - A sequence of calls to addLayerOnTop will match up in the same ordering as the > sequence of paints onto the canvas later, which seems easier / more natural for > developers to reason about. > > I'm happy to look into keeping addLayer instead though. What do you think? Right, I'd specifically like to keep the existing addLayer() ordering. Some people find one order more natural to think about, some people the other, so to tie break I'd just like to not change it.
On 2015/09/28 15:01:48, mtklein wrote: > Right, I'd specifically like to keep the existing addLayer() ordering. Some > people find one order more natural to think about, some people the other, so to > tie break I'd just like to not change it. I've been distracted with other Chromium security stuff, but I hope to get back to this CL next week. I want to ask though: is there anything that would sway you in favor of keeping addLayerOnTop instead of addLayer? I informally polled a handful of people about whether they'd expect adding a layer to a stack of graphic layers would add on top or bottom of the existing layers, and everyone's response so far has been on top. FWIW, I've also prepared proof-of-concept CLs for Chromium for either option: - Using only addLayerOnTop: https://codereview.chromium.org/1363253002/ - Using only addLayer: https://codereview.chromium.org/1393593002/ I feel more confident in the former than the latter since it's marginally smaller and doesn't involve changing API semantics, but I think either should be okay.
On 2015/10/08 22:06:09, mdempsky wrote: > I want to ask though: is there anything that would sway you in favor of keeping > addLayerOnTop instead of addLayer? I informally polled a handful of people > about whether they'd expect adding a layer to a stack of graphic layers would > add on top or bottom of the existing layers, and everyone's response so far has > been on top. mtklein: ping?
On 2015/10/29 at 18:30:35, mdempsky wrote: > On 2015/10/08 22:06:09, mdempsky wrote: > > I want to ask though: is there anything that would sway you in favor of keeping > > addLayerOnTop instead of addLayer? I informally polled a handful of people > > about whether they'd expect adding a layer to a stack of graphic layers would > > add on top or bottom of the existing layers, and everyone's response so far has > > been on top. API changes between Skia and Chrome are a headache, so it's easiest to leave working APIs alone. Let's at least split the interface change and implementation change into two CLs.
On 2015/10/29 18:56:14, mtklein wrote: > API changes between Skia and Chrome are a headache, so it's easiest to leave > working APIs alone. > Let's at least split the interface change and implementation change into two > CLs. I agree on splitting interface/implementation changes. My question is more about what interface change to make. The options are: 1. Remove Builder::addLayer() and keep Builder::addLayerOnTop(); or 2. Remove Builder::addLayerOnTop() and keep Builder::addLayer(). You said you prefer #2, and if you're set on that, I'm willing to accept that and move on. I just wanted to double check first if anything would sway you in favor of #1, since I really think that's the better path forward (people I polled expect add-on-top semantics; simpler SkLayerDrawLooper implementation; fewer Chromium/Blink uses to change).
On 2015/10/29 at 19:07:07, mdempsky wrote: > On 2015/10/29 18:56:14, mtklein wrote: > > API changes between Skia and Chrome are a headache, so it's easiest to leave > > working APIs alone. > > Let's at least split the interface change and implementation change into two > > CLs. > > I agree on splitting interface/implementation changes. My question is more about what interface change to make. The options are: > > 1. Remove Builder::addLayer() and keep Builder::addLayerOnTop(); or > 2. Remove Builder::addLayerOnTop() and keep Builder::addLayer(). > > You said you prefer #2, and if you're set on that, I'm willing to accept that and move on. I just wanted to double check first if anything would sway you in favor of #1, since I really think that's the better path forward (people I polled expect add-on-top semantics; simpler SkLayerDrawLooper implementation; fewer Chromium/Blink uses to change). My preference is 3. Nothing changes. But I'm okay with removing nothing and adding clearer aliases like addLayerOnTop(), addLayerOnBottom(). Does this really matter to you? It's not clear why this change is important.
On 2015/10/29 19:15:40, mtklein wrote: > My preference is 3. Nothing changes. But I'm okay with removing nothing and > adding clearer aliases like addLayerOnTop(), addLayerOnBottom(). > > Does this really matter to you? It's not clear why this change is important. If SkLayerDrawLooper supports adding to just one end or the other, it can be easily and efficiently switched to use a SkTArray instead of the current open-coded linked list implementation. If it continues to support both, then it either needs to make one of those less efficient, or use a more complicated data structure like a deque (and unfortunately SkDeque isn't type safe).
On 2015/10/29 at 19:20:46, mdempsky wrote: > On 2015/10/29 19:15:40, mtklein wrote: > > My preference is 3. Nothing changes. But I'm okay with removing nothing and > > adding clearer aliases like addLayerOnTop(), addLayerOnBottom(). > > > > Does this really matter to you? It's not clear why this change is important. > > If SkLayerDrawLooper supports adding to just one end or the other, it can be easily and efficiently switched to use a SkTArray instead of the current open-coded linked list implementation. > > If it continues to support both, then it either needs to make one of those less efficient, or use a more complicated data structure like a deque (and unfortunately SkDeque isn't type safe). Gotcha. So, why does the implementation change matter? Is this a hot spot?
On 2015/10/29 19:26:05, mtklein wrote: > Gotcha. So, why does the implementation change matter? Is this a hot spot? Just a general security team interest in reducing code complexity when possible. (I'm not aware of it being a hot spot, but I haven't done any measurements either.)
mtklein@google.com changed reviewers: + reed@google.com
Okay, let me pass you over to Mike Reed to talk APIs.
On 2015/10/29 19:49:11, mtklein wrote: > Okay, let me pass you over to Mike Reed to talk APIs. Mike Reed: Ping? To summarize the discussion: - I'm wanting to simplify the implementation of SkLayerDrawLooper, largely for code hygiene. - The implementation can be simplified further if we eliminate either of the addLayer or addLayerOnTop member functions (i.e., only support adding either on top or on bottom, not both). - Looking through the Chromium/Blink code base, it seems somewhat easier to keep addLayerOnTop. This would also slightly simplify the resulting SkLayerDrawLooper implementation, and all of the Chromium/Blink devs I polled expected adding layers on top to be the default behavior if only one operation was supported.
Description was changed from ========== Simplify SkLayerDrawLooper Drop support for adding layers to the bottom, and switch from a linked list to a flat vector of Recs. ========== to ========== Simplify SkLayerDrawLooper DO NOT SUBMIT; PROOF OF CONCEPT / WIP Drop support for adding layers to the bottom, and switch from a linked list to a flat vector of Recs. ==========
On 2015/11/10 21:56:38, mdempsky wrote: > On 2015/10/29 19:49:11, mtklein wrote: > > Okay, let me pass you over to Mike Reed to talk APIs. > > Mike Reed: Ping? > > To summarize the discussion: > - I'm wanting to simplify the implementation of SkLayerDrawLooper, largely for > code hygiene. > - The implementation can be simplified further if we eliminate either of the > addLayer or addLayerOnTop member functions (i.e., only support adding either on > top or on bottom, not both). > - Looking through the Chromium/Blink code base, it seems somewhat easier to > keep addLayerOnTop. This would also slightly simplify the resulting > SkLayerDrawLooper implementation, and all of the Chromium/Blink devs I polled > expected adding layers on top to be the default behavior if only one operation > was supported. (Oh, and this particular CL is not meant to be submitted and has known issues like using std::vector instead of SkTArray. It's just to demonstrate the intended cleanup.)
I don't see much incentive to change the impl, but I'm fine if you want to eliminate addLayer and just use addLayerOnTop, assuming you can fix all the call sites. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
