Bug 154771 - Fix the behavior of reflecting IDL attributes of type unsigned long
Summary: Fix the behavior of reflecting IDL attributes of type unsigned long
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: History (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2016-02-26 20:47 PST by Chris Dumez
Modified: 2016-02-27 15:48 PST (History)
8 users (show)

See Also:


Attachments
Patch (21.20 KB, patch)
2016-02-26 21:17 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.05 KB, patch)
2016-02-26 22:07 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.