Bug 226810

Created attachment 431060 [details] Patch

Comment on attachment 431060 [details] Patch Hi, I think this patch is ready for review now:) It applies HTMLDimension to other length parsing scenarios. Please take a look, thanks!

Comment on attachment 431060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431060&action=review > Source/WebCore/html/HTMLElement.cpp:1019 > +void HTMLElement::addHTMLLengthToStyleWithConfigs(MutableStyleProperties& style, CSSPropertyID propertyID, const String& value, AllowPercentage allowPercentage, UseCSSPXAsUnitType useCSSPX, IsMultiLength isMultiLengh) Typo here in isMultiLengh Not thrilled with the word "configs" here. I think the function name doesn’t have to change just because we have three more arguments; it can just be an overload. Think of the arguments as having default values. > Source/WebCore/html/HTMLElement.h:155 > + enum class AllowPercentage : bool { No, Yes }; > + enum class UseCSSPXAsUnitType : bool { No, Yes }; > + enum class IsMultiLength : bool { No, Yes }; > + void addHTMLLengthToStyleWithConfigs(MutableStyleProperties&, CSSPropertyID, const String& value, AllowPercentage, UseCSSPXAsUnitType, IsMultiLength); These all can and should be private instead of protected. Also not sure that these options are best represented as three separate booleans, but if we make that private it’s easy to change it later. This function should take a StringView, not a const String&. > Source/WebCore/html/HTMLElement.h:159 > void addHTMLLengthToStyle(MutableStyleProperties&, CSSPropertyID, const String& value); > + void addHTMLMultiLengthToStyle(MutableStyleProperties&, CSSPropertyID, const String& value); > + void addHTMLPixelsToStyle(MutableStyleProperties&, CSSPropertyID, const String& value); > + void addHTMLNumberToStyle(MutableStyleProperties&, CSSPropertyID, const String& value); These functions should take StringView, not const String&.

Comment on attachment 431060 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431060&action=review Hi Darin, Thanks for the review! >> Source/WebCore/html/HTMLElement.cpp:1019 >> +void HTMLElement::addHTMLLengthToStyleWithConfigs(MutableStyleProperties& style, CSSPropertyID propertyID, const String& value, AllowPercentage allowPercentage, UseCSSPXAsUnitType useCSSPX, IsMultiLength isMultiLengh) > > Typo here in isMultiLengh > > Not thrilled with the word "configs" here. I think the function name doesn’t have to change just because we have three more arguments; it can just be an overload. Think of the arguments as having default values. Got it! Done, thanks! >> Source/WebCore/html/HTMLElement.h:155 >> + void addHTMLLengthToStyleWithConfigs(MutableStyleProperties&, CSSPropertyID, const String& value, AllowPercentage, UseCSSPXAsUnitType, IsMultiLength); > > These all can and should be private instead of protected. Also not sure that these options are best represented as three separate booleans, but if we make that private it’s easy to change it later. > > This function should take a StringView, not a const String&. Done! >> Source/WebCore/html/HTMLElement.h:159 >> + void addHTMLNumberToStyle(MutableStyleProperties&, CSSPropertyID, const String& value); > > These functions should take StringView, not const String&. Ah, done, thanks!

Created attachment 431198 [details] Patch

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

<rdar://problem/79205778>