Bug 154845 – Have parseHTMLInteger() / parseHTMLNonNegativeInteger() use WTF::Optional

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Have parseHTMLInteger() / parseHTMLNonNegativeInteger() use WTF::Optional

[Chris Dumez]

Created attachment 272545 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 272545 [details]
Patch

Clearing flags on attachment: 272545

Committed r197389: <http://trac.webkit.org/changeset/197389>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 272605 [details]
Fix review comments

[Chris Dumez]

Reopening to fix review comments.

[Chris Dumez]

Created attachment 272609 [details]
Fix review comments

[Chris Dumez]

Comment on attachment 272609 [details]
Fix review comments

Clearing flags on attachment: 272609

Committed r197449: <http://trac.webkit.org/changeset/197449>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.