Bug 237244

<rdar://problem/89524668>

Created attachment 453331 [details] Patch

Comment on attachment 453331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453331&action=review > Source/WebCore/rendering/style/RenderStyle.h:617 > + UserModify effectiveUserModify() const { return effectiveInert() ? UserModify::ReadOnly : userModify(); } Maybe these "effective" functions should be in a separate paragraph, rather than being interspersed with the getters for the actual stored values? Some day we might even automatically generate or do something else like that for the stored values, and the effective family of functions seems like a different thing.

Comment on attachment 453331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453331&action=review >> Source/WebCore/rendering/style/RenderStyle.h:617 >> + UserModify effectiveUserModify() const { return effectiveInert() ? UserModify::ReadOnly : userModify(); } > > Maybe these "effective" functions should be in a separate paragraph, rather than being interspersed with the getters for the actual stored values? > > Some day we might even automatically generate or do something else like that for the stored values, and the effective family of functions seems like a different thing. Right now, this just follows the long tail of effectiveAppearance/effectiveDisplay/effectiveZoom/effectiveTouchActions/etc. which also have their getter live next to the actual value. I find that slightly better since someone who might look at userModify() or such, will tell themselves: "maybe effectiveUserModify() is the thing I want to use". This area has some potential for clean up in general, in some places, we use the prefix "used" e.g. usedZindex/usedTransformStyle3D/usedFloat/usedClear/etc. The line is so thin between "used" and "effective" that even Blink consistently uses "used". There is also "resolved" for some flex/grid related stuff, but that's slightly different since they don't necessarily refer directly to CSS properties. I do think it's worth having discussions with the relevant stakeholders about these things, since this repeating pattern might be an potential area for automation/refactoring, etc. but this bug is probably not the right place to act :)

Committed r290564 (247842@main): <https://commits.webkit.org/247842@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453331 [details].

Comment on attachment 453331 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453331&action=review >>> Source/WebCore/rendering/style/RenderStyle.h:617 >>> + UserModify effectiveUserModify() const { return effectiveInert() ? UserModify::ReadOnly : userModify(); } >> >> Maybe these "effective" functions should be in a separate paragraph, rather than being interspersed with the getters for the actual stored values? >> >> Some day we might even automatically generate or do something else like that for the stored values, and the effective family of functions seems like a different thing. > > Right now, this just follows the long tail of effectiveAppearance/effectiveDisplay/effectiveZoom/effectiveTouchActions/etc. which also have their getter live next to the actual value. I find that slightly better since someone who might look at userModify() or such, will tell themselves: "maybe effectiveUserModify() is the thing I want to use". > > This area has some potential for clean up in general, in some places, we use the prefix "used" e.g. usedZindex/usedTransformStyle3D/usedFloat/usedClear/etc. The line is so thin between "used" and "effective" that even Blink consistently uses "used". > > There is also "resolved" for some flex/grid related stuff, but that's slightly different since they don't necessarily refer directly to CSS properties. > > I do think it's worth having discussions with the relevant stakeholders about these things, since this repeating pattern might be an potential area for automation/refactoring, etc. but this bug is probably not the right place to act :) Sure, I am suggesting moving effectiveAppearance/effectiveDisplay/effectiveZoom/effectiveTouchActions/etc. Was that unclear? Adding one more effectiveXXX function does seem like a good time to move the others; doesn’t seem like an excessive thing to do. If you prefer not to do it while adding this item, that’s OK, but it is what I was suggesting. I’m not sure why you’re expanding the conversation to those other cleanup ideas, though, as good as they might be. I wanted to suggest one step directly related to what you are doing, adding another effectiveXXX so that now there are more than ever before. It helped me notice what seems to be an increasingly poor pattern of interspersing these.; I agree there are lots of other things that can be done to this class. We have had great success doing these things incrementally in WebKit in the past, and I hope to continue that tradition.

(In reply to Darin Adler from comment #6) > Comment on attachment 453331 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453331&action=review > > >>> Source/WebCore/rendering/style/RenderStyle.h:617 > >>> + UserModify effectiveUserModify() const { return effectiveInert() ? UserModify::ReadOnly : userModify(); } > >> > >> Maybe these "effective" functions should be in a separate paragraph, rather than being interspersed with the getters for the actual stored values? > >> > >> Some day we might even automatically generate or do something else like that for the stored values, and the effective family of functions seems like a different thing. > > > > Right now, this just follows the long tail of effectiveAppearance/effectiveDisplay/effectiveZoom/effectiveTouchActions/etc. which also have their getter live next to the actual value. I find that slightly better since someone who might look at userModify() or such, will tell themselves: "maybe effectiveUserModify() is the thing I want to use". > > > > This area has some potential for clean up in general, in some places, we use the prefix "used" e.g. usedZindex/usedTransformStyle3D/usedFloat/usedClear/etc. The line is so thin between "used" and "effective" that even Blink consistently uses "used". > > > > There is also "resolved" for some flex/grid related stuff, but that's slightly different since they don't necessarily refer directly to CSS properties. > > > > I do think it's worth having discussions with the relevant stakeholders about these things, since this repeating pattern might be an potential area for automation/refactoring, etc. but this bug is probably not the right place to act :) > > Sure, I am suggesting moving > effectiveAppearance/effectiveDisplay/effectiveZoom/effectiveTouchActions/etc. > Was that unclear? I don't necessarily agree that's better to have all effective* properties in one place. The first thing you come across when searching code related to a certain property are the non-effective versions. Currently, when you consult the code, you see right away that there is an effective version of it, which makes you immediately question which version you should use. If all effective versions were all in one block, away from the non-effective versions, you wouldn't necessarily question that, and may end up using the wrong version. Things would probably be different if all properties had an effective version, but that's not the case atm. I do think that relying on where the methods are placed in the header file for good understanding of the code isn't great, and this is why I think a wider conversation makes sense.

(In reply to Tim Nguyen (:ntim) from comment #7) > I don't necessarily agree that's better to have all effective* properties in > one place. OK.