Have parseHTMLInteger() / parseHTMLNonNegativeInteger() use WTF::Optional
Created attachment 272545 [details] Patch
Comment on attachment 272545 [details] Patch Clearing flags on attachment: 272545 Committed r197389: <http://trac.webkit.org/changeset/197389>
All reviewed patches have been landed. Closing bug.
Comment on attachment 272545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272545&action=review > Source/WebCore/html/HTMLElement.cpp:137 > + auto optionalBorderWidth = value.isEmpty() ? Nullopt : parseHTMLNonNegativeInteger(value); > + return optionalBorderWidth.valueOr(hasTagName(tableTag) ? 1 : 0); Since parseHTMLNonNegativeInteger already returns Nullopt when passed an empty string, there is no need the explicit isEmpty check here; we should remove that. I’ll also note that doing work inside the expression passed to valueOr is not the same as using an explicit “if”; the hasTagName check is always done in the new code, and was only conditionally done in the old code. > Source/WebCore/html/HTMLElement.cpp:457 > - int tabIndex = 0; > if (value.isEmpty()) > clearTabIndexExplicitlyIfNeeded(); > - else if (parseHTMLInteger(value, tabIndex)) { > + else if (auto optionalTabIndex = parseHTMLInteger(value)) { > // Clamp tab index to a 16-bit value to match Firefox's behavior. > - setTabIndexExplicitly(std::max(-0x8000, std::min(tabIndex, 0x7FFF))); > + setTabIndexExplicitly(std::max(-0x8000, std::min(optionalTabIndex.value(), 0x7FFF))); > } The behavior here is unlikely to be correct. No attribute at all and the empty string both do clearTabIndexExplicitlyIfNeeded. But a string containing, say, just a space character, does not. I would be surprised if that is actually helpful behavior. We probably don’t have sufficient tests for dynamically changing to detect this mistake. > Source/WebCore/html/HTMLTextAreaElement.cpp:429 > + auto optionalMaxLength = parseHTMLNonNegativeInteger(fastGetAttribute(maxlengthAttr)); > + if (!optionalMaxLength) > + return -1; > + return optionalMaxLength.value(); Annoying that we can’t use valueOr(-1) here. It’s a little peculiar that parseHTMLNonNegativeInteger actually returns a non-negative int, but we use the type unsigned. I like using unsigned for things guaranteed to not be negative, but it seems to get a little messy. > Source/WebCore/html/parser/HTMLParserIdioms.h:126 > ASSERT(value > 0 && value <= maxHTMLNonNegativeInteger); Normally we’d break this up into two separate assertions, rather than using &&. Also, I think we should assert that defaultValue is non-zero and assert that it is <= maxHTMLNonNegativeInteger, rather than just asserting that about the result of the function. After all, it’s unlikely that parseHTMLNonNegativeInteger will get it wrong, and much more likely that the person passes in an inappropriate default value. In fact, I think we probably only need to assert these things about the default value. No real need to assert that parseHTMLNonNegativeInteger did its job. > Source/WebCore/html/parser/HTMLParserIdioms.h:139 > ASSERT(value <= maxHTMLNonNegativeInteger); Same comment about this assertion. We should be asserting about the value of defaultValue, not about our ultimate function return result, because we know that parseHTMLNonNegativeInteger will do its job, but we don’t know about the caller’s passed in number. > Source/WebCore/svg/SVGElement.cpp:521 > + else if (auto optionalTabIndex = parseHTMLInteger(value)) { Not sure we really need the word optional in the name of this local.
Comment on attachment 272545 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=272545&action=review >> Source/WebCore/html/HTMLElement.cpp:137 >> + return optionalBorderWidth.valueOr(hasTagName(tableTag) ? 1 : 0); > > Since parseHTMLNonNegativeInteger already returns Nullopt when passed an empty string, there is no need the explicit isEmpty check here; we should remove that. > > I’ll also note that doing work inside the expression passed to valueOr is not the same as using an explicit “if”; the hasTagName check is always done in the new code, and was only conditionally done in the old code. Ok, will fix both issues. >> Source/WebCore/html/HTMLElement.cpp:457 >> } > > The behavior here is unlikely to be correct. No attribute at all and the empty string both do clearTabIndexExplicitlyIfNeeded. But a string containing, say, just a space character, does not. I would be surprised if that is actually helpful behavior. We probably don’t have sufficient tests for dynamically changing to detect this mistake. Ok, I'll take a look at this separately as it likely needs test coverage. >> Source/WebCore/html/HTMLTextAreaElement.cpp:429 >> + return optionalMaxLength.value(); > > Annoying that we can’t use valueOr(-1) here. It’s a little peculiar that parseHTMLNonNegativeInteger actually returns a non-negative int, but we use the type unsigned. I like using unsigned for things guaranteed to not be negative, but it seems to get a little messy. Agreed, let's return an Optional<int> and use valueOr(-1). >> Source/WebCore/html/parser/HTMLParserIdioms.h:126 >> ASSERT(value > 0 && value <= maxHTMLNonNegativeInteger); > > Normally we’d break this up into two separate assertions, rather than using &&. > > Also, I think we should assert that defaultValue is non-zero and assert that it is <= maxHTMLNonNegativeInteger, rather than just asserting that about the result of the function. After all, it’s unlikely that parseHTMLNonNegativeInteger will get it wrong, and much more likely that the person passes in an inappropriate default value. In fact, I think we probably only need to assert these things about the default value. No real need to assert that parseHTMLNonNegativeInteger did its job. Ok, I'll add assertions for the defaultValue. However, I'd still like to keep these to be safe. >> Source/WebCore/svg/SVGElement.cpp:521 >> + else if (auto optionalTabIndex = parseHTMLInteger(value)) { > > Not sure we really need the word optional in the name of this local. I find it confusing otherwise, especially with the use of auto. if (auto tabIndex = parseHTMLInteger(value)) // If you're not familiar with parseHTMLInteger, you may assume that we're here because tabIndex is not zero. Maybe I should do: if (Optional<int> tabIndex = parseHTMLInteger(value)) Then I would find it clear.
Created attachment 272605 [details] Fix review comments
Reopening to fix review comments.
Created attachment 272609 [details] Fix review comments
Comment on attachment 272609 [details] Fix review comments Clearing flags on attachment: 272609 Committed r197449: <http://trac.webkit.org/changeset/197449>