Bug 209985

Created attachment 395538 [details] Patch

Comment on attachment 395538 [details] Patch Ready for review now.

Adding Antti since he’s been reviewing this series.

Comment on attachment 395538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395538&action=review Wish the patch was more focused on what it says in the title, with unrelated refactoring done separately. > Source/WebCore/dom/DocumentMarker.h:105 > + , DictationData // DictationAlternatives > +#if PLATFORM(IOS_FAMILY) > + , Vector<String> // DictationPhraseWithAlternatives > + , RetainPtr<id> // DictationResult > +#endif > + , RefPtr<Node> // DraggedContent I'm not sure removing the types and then having to comment here what the variants are is an improvement. Call sites end up confusing too. > Source/WebCore/dom/DocumentMarker.h:120 > - void clearData() { m_data = false; } > + void clearData() { m_data = String { }; } You might consider using WTF::Monostate for the empty Variant case. It can be cleaner to use a state that is separate from any actual values. > Source/WebCore/dom/DocumentMarkerController.cpp:629 > + if (auto renderer = node.renderer()) > + renderer->repaint(); I think we still usually mark plain pointers with auto* > Source/WebCore/dom/Node.h:206 > - virtual bool isCharacterDataNode() const { return false; } > + bool isCharacterDataNode() const { return !isContainerNode() && (isTextNode() || virtualIsCharacterData()); } Nice ninja optimization. > Source/WebCore/dom/SimpleRange.h:59 > +IntersectingNodeRange intersectingNodes(const SimpleRange&); This is nice. > Source/WebCore/rendering/RenderObject.h:156 > - // Convenience function for getting to the nearest enclosing box of a RenderObject. > + // Nearest enclosing box. > WEBCORE_EXPORT RenderBox& enclosingBox() const; Event the compressed comment doesn't really add anything (and if it did, the right action would be to rename the function). > Source/WebCore/rendering/RenderObject.h:172 > + // Forbid calls to setNeedsLayout() during its lifetime. > + class SetLayoutNeededForbiddenScope; "Forbid" is vague. Can the type be renamed to say "assert"? At least the comment should mention this just enables an assert.

(In reply to Antti Koivisto from comment #10) > Wish the patch was more focused on what it says in the title, with unrelated > refactoring done separately. I’ll try for that more in the future. My next patch has many bits of refactoring in it, and I will break it into pieces rather than landing all at once.

Comment on attachment 395538 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395538&action=review >> Source/WebCore/dom/DocumentMarker.h:105 >> + , RefPtr<Node> // DraggedContent > > I'm not sure removing the types and then having to comment here what the variants are is an improvement. Call sites end up confusing too. Even with the structures, it wasn't clear which variant value was for which document marker type, except for the structures that happened to be named the same as the types. The real solution would be to get rid of the enum for the type and use structures only for the types. But this wouldn’t work in an OptionSet. There’s no great way to combine a Variant and an enum. I wish there was. The comments are to address that. The structures were wrong, for example DictationAlternativesData had two data members, but only one each was used for two different marker types. I removed two of them because they were wrong. I could bring back either structures or named types to make call sites a little easier to read. The main thing that helped make the call sites easy to read before was all the convenience functions, which help to hide the ugliness of using a Variant. I could add those back, but I wouldn't put them in this header because then there’s a lot of conditional code. Your comment is definitely valid! However, I am tempted to leave this exactly as-is, for now at least. >> Source/WebCore/dom/DocumentMarker.h:120 >> + void clearData() { m_data = String { }; } > > You might consider using WTF::Monostate for the empty Variant case. It can be cleaner to use a state that is separate from any actual values. I did consider it. But since the "basic" type here is String, null string works so well for this. I agree Monostate is cleaner, but not enough better that it seemed worth it to me. >> Source/WebCore/dom/DocumentMarkerController.cpp:629 >> + renderer->repaint(); > > I think we still usually mark plain pointers with auto* I am willing to do that. But I have noticed as I have been refactoring to turn plain pointer return values in RefPtr and Ref (as requested by Ryosuke) that having the * will require touching many call sites that otherwise would not have be touched. Maybe the real question is whether we will be doing that or not. If not, then the * can help remind people something is a raw pointer. >> Source/WebCore/dom/Node.h:206 >> + bool isCharacterDataNode() const { return !isContainerNode() && (isTextNode() || virtualIsCharacterData()); } > > Nice ninja optimization. Should have just landed this separately. Had it sitting in the SimpleRange.cpp source file waiting to be moved up. >> Source/WebCore/dom/SimpleRange.h:59 >> +IntersectingNodeRange intersectingNodes(const SimpleRange&); > > This is nice. Thanks. I’m trying to make the idiom better since I have to touch the code that was using live ranges. A safer approach would be to replicate the old crufty way Range worked, but I think it’s worth the risk. >> Source/WebCore/rendering/RenderObject.h:156 >> WEBCORE_EXPORT RenderBox& enclosingBox() const; > > Event the compressed comment doesn't really add anything (and if it did, the right action would be to rename the function). Will remove it. >> Source/WebCore/rendering/RenderObject.h:172 >> + class SetLayoutNeededForbiddenScope; > > "Forbid" is vague. Can the type be renamed to say "assert"? At least the comment should mention this just enables an assert. I will change the comment. Totally willing to rename too (not that I named these). Here are the current names: isSetNeedsLayoutForbidden setNeedsLayoutIsForbidden m_setNeedsLayoutForbidden SetLayoutNeededForbiddenScope SetLayoutNeededForbiddenScope::m_preexistingForbidden

Committed r259575: <https://trac.webkit.org/changeset/259575>

<rdar://problem/61347856>

> The main thing that helped make the call sites easy to read before was all > the convenience functions, which help to hide the ugliness of using a > Variant. I could add those back, but I wouldn't put them in this header > because then there’s a lot of conditional code. Accessors seem to work well when not going all-in with WTF::switchOn etc. > Your comment is definitely valid! > > However, I am tempted to leave this exactly as-is, for now at least. That is ok too. > I will change the comment. Totally willing to rename too (not that I named > these). Here are the current names: I know, but this patch removes the debug ifdefs, so it is no longer clear this is debug code.

(In reply to Antti Koivisto from comment #15) > > The main thing that helped make the call sites easy to read before was all > > the convenience functions, which help to hide the ugliness of using a > > Variant. I could add those back, but I wouldn't put them in this header > > because then there’s a lot of conditional code. > > Accessors seem to work well when not going all-in with WTF::switchOn etc. I agree that since WTF::get is so ugly it’s easier to read code that wraps it in a function. The getters do that, but maybe I can find another place for the helper functions that avoid concentrating all the code in one place. It should be easy to add another document marker type without making the code hard to read. Before my patch this was full of unused features, and I feel that the complexity of the header file made it hard to see that. I also think that DocumentMarker would be better as a struct rather than a class. > > I will change the comment. Totally willing to rename too (not that I named > > these). Here are the current names: > > I know, but this patch removes the debug ifdefs, so it is no longer clear > this is debug code. Got it. Maybe you can help me come up with the new names. I will definitely change them if we can decide on good ones. How about these? isSetNeedsLayoutForbidden -> shouldAssertOnSetNeedsLayout setNeedsLayoutIsForbidden -> setAssertOnSetNeedsLayout m_setNeedsLayoutForbidden -> m_shouldAssertOnSetNeedsLayout SetLayoutNeededForbiddenScope -> SetAssertOnSetNeedsLayout SetLayoutNeededForbiddenScope::m_preexistingForbidden -> m_oldValue