Bug 154771

Summary: Fix the behavior of reflecting IDL attributes of type unsigned long
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: HistoryAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, commit-queue, darin, esprehn+autocc, gyuyoung.kim, kangil.han, rniwa, sam
Priority: P2 Keywords: WebExposed
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Dumez 2016-02-26 20:47:29 PST
Fix the behavior of reflecting IDL attributes of type unsigned long to align with the specification:
- https://html.spec.whatwg.org/multipage/infrastructure.html#reflecting-content-attributes-in-idl-attributes
- https://html.spec.whatwg.org/multipage/infrastructure.html#rules-for-parsing-non-negative-integers

There are several issues with the current implementation:
- Upon getting, the value returned must be in the range 0 to 2147483647. Otherwise, we must return the default value (0 unless specified otherwise). We currently return values in the range 0 to 4294967295 instead.
- Upon setting, we must set the content attribute to the default value (0 unless specified otherwise) if the input value is not in the range 0 to 2147483647. We currently allow values in the range 0 to 4294967295 instead.
- "-0" is not recognized as a valid unsigned integer
Comment 1 Chris Dumez 2016-02-26 20:48:34 PST
Firefox and Chrome match the specification.
Comment 2 Chris Dumez 2016-02-26 21:17:48 PST
Created attachment 272399 [details]
Patch
Comment 3 Ryosuke Niwa 2016-02-26 21:31:30 PST
Comment on attachment 272399 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272399&action=review

> Source/WebCore/html/parser/HTMLParserIdioms.h:123
> +    ASSERT(value > 0 && value <= 2147483647);

Can we use 0x7FFFFFFF instead and define a static const somewhere?
e.g. static const MaxHTMLNonNegativeNumber = 0x7FFFFFFF.
Comment 4 Chris Dumez 2016-02-26 21:50:45 PST
(In reply to comment #3)
> Comment on attachment 272399 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272399&action=review
> 
> > Source/WebCore/html/parser/HTMLParserIdioms.h:123
> > +    ASSERT(value > 0 && value <= 2147483647);
> 
> Can we use 0x7FFFFFFF instead and define a static const somewhere?
> e.g. static const MaxHTMLNonNegativeNumber = 0x7FFFFFFF.

I find 2147483647 a lot more readable than 0x7FFFFFFF personally. It also matches what is in the HTML and the Web IDL specs.
Comment 5 Ryosuke Niwa 2016-02-26 21:54:05 PST
(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 272399 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=272399&action=review
> > 
> > > Source/WebCore/html/parser/HTMLParserIdioms.h:123
> > > +    ASSERT(value > 0 && value <= 2147483647);
> > 
> > Can we use 0x7FFFFFFF instead and define a static const somewhere?
> > e.g. static const MaxHTMLNonNegativeNumber = 0x7FFFFFFF.
> 
> I find 2147483647 a lot more readable than 0x7FFFFFFF personally. It also
> matches what is in the HTML and the Web IDL specs.

Really!? 2147483647 doesn't tell me anything about why it's significant whereas 0x7FFFFFFF would tell us exactly why that value is picked.
Comment 6 Chris Dumez 2016-02-26 22:07:24 PST
Created attachment 272405 [details]
Patch
Comment 7 Chris Dumez 2016-02-26 22:09:30 PST
Comment on attachment 272405 [details]
Patch

Clearing flags on attachment: 272405

Committed r197237: <http://trac.webkit.org/changeset/197237>
Comment 8 Chris Dumez 2016-02-26 22:09:36 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Darin Adler 2016-02-27 15:48:54 PST
(In reply to comment #4)
> I find 2147483647 a lot more readable than 0x7FFFFFFF personally. It also
> matches what is in the HTML and the Web IDL specs.

I am not sure what you mean by readable, but I think that’s not really possible. For example, what if I wrote this:

    2147383647

Would you be able to spot that value is not 2^31-1? It’s true that the same problem can happen with:

    0x7FFFFFF

But I, at least, can see the difference, whereas with the decimal number it’s just always unsafe, could have one digit wrong and we’d never see it.