Bug 49786 - Implement (non-EventListener) marquee IDL attributes from HTML5.
: Implement (non-EventListener) marquee IDL attributes from HTML5.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To: Andy Estes
: InRadar
Depends on:
Blocks: 49788 50434
  Show dependency treegraph
 
Reported: 2010-11-18 23:24 PST by Andy Estes
Modified: 2010-12-02 19:42 PST (History)
6 users (show)

See Also:


Attachments
Patch (13.75 KB, patch)
2010-11-18 23:50 PST, Andy Estes
no flags Details | Formatted Diff | Diff
Patch (23.92 KB, patch)
2010-12-01 15:51 PST, Andy Estes
darin: review+
Details | Formatted Diff | Diff
Fix clang build (1.46 KB, patch)
2010-12-02 19:02 PST, Yuta Kitamura
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2010-11-18 23:24:08 PST
Implement (non-EventListener) marquee IDL attributes from HTML5.
Comment 1 Andy Estes 2010-11-18 23:37:45 PST
<rdar://problem/7863203>
Comment 2 Andy Estes 2010-11-18 23:50:29 PST
Created attachment 74359 [details]
Patch
Comment 3 Darin Adler 2010-11-19 10:37:35 PST
Comment on attachment 74359 [details]
Patch

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

This seems OK, but could easily be event better.

> WebCore/html/HTMLMarqueeElement.cpp:135
> +    if (NamedNodeMap* namedNodeMap = attributes()) {
> +        if (Attribute* scrollAmount = namedNodeMap->getAttributeItem(scrollamountAttr))
> +            return scrollAmount->value().string().toUInt();
> +    }

This should be written like this:

    const AtomicString& scrollamount = fastGetAttribute(scrollAmountAttr);
    if (!scrollAmount.isNull())
        return scrollAmount.string().toUInt();

Also, I don’t see the test cases about edge cases of parsing that would verify whether toUInt is appropriate, such as garbage after a numeric string or the empty string, a string with a non-digit in it a string with a negative number in it a string with a number but some leading whitespace, a string with the largest integer in it, or a string with the largest integer plus one. Depending on how those edge cases should work, this code might or might not be correct.

> WebCore/html/HTMLMarqueeElement.cpp:149
> +    if (NamedNodeMap* namedNodeMap = attributes()) {
> +        if (Attribute* scrollDelay = namedNodeMap->getAttributeItem(scrolldelayAttr))
> +            return scrollDelay->value().string().toUInt();
> +    }

Same thing here. It’s not right to use attributes(). You should use fastGetAttribute instead.

I’d like to see more test cases about parsing edge cases as I mentioned above.

> WebCore/html/HTMLMarqueeElement.cpp:166
> +    if (NamedNodeMap* namedNodeMap = attributes()) {
> +        if (Attribute* loop = namedNodeMap->getAttributeItem(loopAttr)) {
> +            int loopVal = loop->value().string().toInt();
> +            return (loopVal > 0 ? loopVal : -1);
> +        }
> +    }
> +    return -1;

This can be written like this:

    int loopValue = fastGetAttribute(loopAttr).string().toInt();
    return loopValue > 0 ? loopValue : -1;

I’d like to see more test cases about parsing edge cases as I mentioned above.

> WebCore/html/HTMLMarqueeElement.cpp:172
> +    if (loop > 0 || loop == -1)
> +        setIntegralAttribute(loopAttr, loop);

The loop == -1 case is not covered by the test case.

> WebCore/html/HTMLMarqueeElement.idl:27
> +        attribute [Reflect=bgcolor] DOMString bgColor;

No need for "=bgcolor" here. [Reflect] handles the case difference automatically. You can see examples of this in other files such as HTMLBodyElement.idl.

> WebCore/html/HTMLMarqueeElement.idl:34
> +        attribute [Reflect=truespeed] boolean trueSpeed;

No need for "=truespeed" here. [Reflect] handles the case difference automatically.

> LayoutTests/fast/html/script-tests/marquee-element.js:1
> +description('Various tests for the marquee element.');

Does this test pass on IE?
Comment 4 Alexey Proskuryakov 2010-11-19 11:37:53 PST
>    const AtomicString& scrollamount = fastGetAttribute(scrollAmountAttr);
>    if (!scrollAmount.isNull())
>        return scrollAmount.string().toUInt();

These are mapped presentation attributes, is this the right solution to get them from attribute map and then duplicate code that applies a default? When looking at this patch, I thought that it wasn't, and there are very few calls to fastGetAttribute in DOM code, but I'm not sure what we usually do. 

> LayoutTests/fast/html/script-tests/marquee-element.js:1

I'd really love for everyone to stop splitting tests into separate JS files. It only causes inconvenience when you can't see test code directly.
Comment 5 Darin Adler 2010-11-19 12:14:31 PST
(In reply to comment #4)
> >    const AtomicString& scrollamount = fastGetAttribute(scrollAmountAttr);
> >    if (!scrollAmount.isNull())
> >        return scrollAmount.string().toUInt();
> 
> These are mapped presentation attributes, is this the right solution to get them from attribute map and then duplicate code that applies a default? When looking at this patch, I thought that it wasn't, and there are very few calls to fastGetAttribute in DOM code, but I'm not sure what we usually do. 

Older code uses getAttribute. There are existing functions like this one, such as:

    HTMLTextAreaElement::maxLength
    HTMLVideoElement::height
    HTMLVideoElement::width

If this element was in HTML5 I’m sure looking there could give us a clearer idea of whether these can be implemented as something that simply reflects the content attribute. Is it in HTML5?

> > LayoutTests/fast/html/script-tests/marquee-element.js:1
> 
> I'd really love for everyone to stop splitting tests into separate JS files. It only causes inconvenience when you can't see test code directly.

You should bring this up on webkit-dev. There’s some benefit in not writing boilerplate over and over again which can be achieved multiple ways, and also major cost in having tests that are spread across two files instead of one self-contained file or one file that references some standard library file. But there’s also confusion about the notion of pure-JavaScript tests that can be run outside run-webkit-tests, which is where the whole script-tests thing began but has never actually been implemented.
Comment 6 Alexey Proskuryakov 2010-11-19 12:23:55 PST
> If this element was in HTML5 I’m sure looking there could give us a clearer idea of whether these can be implemented as something that simply reflects the content attribute. Is it in HTML5?

Yes. HTML5 says "the scrollAmount IDL attribute must reflect the scrollamount content attribute. The default value is 6." This is a relatively uncommon case of reflection, but it is discussed in section 2.8.1 "Reflecting content attributes in IDL attributes."

> You should bring this up on webkit-dev.

IIRC I have, and I even convinced some people back then. Trying personal attacks now :)
Comment 7 Alexey Proskuryakov 2010-11-19 12:24:48 PST
> This is a relatively uncommon case of reflection

Meaning that I couldn't easily identify a pattern in how we implement those. It seemed that we usually had a member variable in the DOM class.
Comment 8 Andy Estes 2010-11-30 12:46:24 PST
Thanks for the feedback Darin. Testing setting an unsigned property to a negative value demonstrated that this patch is certainly wrong. Unfortunately, simply making a reflected property 'unsigned' in the IDL does not mean that the generated code handles it according to HTML5's 'rules for parsing non-negative integers' (2.5.4.1). Specifically, negative values just get casted to unsigned instead of raising an error.

The way we handle unsigned properties in other elements (e.g. HTMLInputElement::maxLength) is to actually make the property signed in the IDL and write a custom setter that raises INDEX_SIZE_ERR if the value is less than zero.

It seems like it would be better to have the bindings generator handle this so we don't have to write custom code each time we add a new unsigned IDL property.
Comment 9 Andy Estes 2010-12-01 15:51:10 PST
Created attachment 75329 [details]
Patch
Comment 10 Andy Estes 2010-12-01 16:01:58 PST
(In reply to comment #9)
> Created an attachment (id=75329) [details]
> Patch

This patch address Darin's and Alexey's review comments. It also handles certain edge cases correctly, as opposed to the previous patch.

Darin, you asked if the test cases pass in IE, and the answer is yes with a few exceptions. IE disagrees with how HTML5 specs marquee in two cases AFAICT:

1) For scrollAmount, scrollDelay and loop, IE does not allow the content attribute value to contain trailing non-numeric characters, although such characters are allowed according to HTML5's "rules for parsing non-negative integers" (section 2.5.4.1). IE will return the default value for these attributes if non-numeric characters are encountered, but HTML5 says to parse up to the first non-number and return that value.

2) For scrollAmount, scrollDelay and loop, if a value is specified that exceeds 2^31-1, IE will return 2^31-1. HTML5 says that for reflected unsigned longs, if a value is larger than 2^31-1, the default value should be returned (section 2.8.1).

In both these cases, I chose to follow HTML5 rather than IE, which causes a handful of test failures in IE. Since my understanding of marquee is that it started as a proprietary element in IE, I'm a little surprised HTML5 specs it in a way that disagrees with IE's behavior. Perhaps this is a bug in the spec.
Comment 11 Andy Estes 2010-12-02 13:15:15 PST
http://trac.webkit.org/changeset/73189
Comment 12 WebKit Review Bot 2010-12-02 13:22:49 PST
http://trac.webkit.org/changeset/73189 might have broken Qt Linux Release minimal
Comment 13 Yuta Kitamura 2010-12-02 18:54:19 PST
Hi, it seemed your change broke Chromium's clang build:

>/b/build/slave/Chromium_Mac_Debug_Clang/build/src/third_party/WebKit/WebCore/WebCore.gyp/../html/HTMLMarqueeElement.cpp:150:35:error: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
>    return ok && scrollDelay >= 0 ? scrollDelay : RenderStyle::initialMarqueeSpeed();
>                                  ^ ~~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>1 error generated.

I think RenderStyle::initialMarqueeSpeed() should return int.
Comment 14 Andy Estes 2010-12-02 18:58:48 PST
(In reply to comment #13)
> Hi, it seemed your change broke Chromium's clang build:
> 
> >/b/build/slave/Chromium_Mac_Debug_Clang/build/src/third_party/WebKit/WebCore/WebCore.gyp/../html/HTMLMarqueeElement.cpp:150:35:error: operands of ? are integers of different signs: 'int' and 'unsigned int' [-Wsign-compare]
> >    return ok && scrollDelay >= 0 ? scrollDelay : RenderStyle::initialMarqueeSpeed();
> >                                  ^ ~~~~~~~~~~~   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >1 error generated.
> 
> I think RenderStyle::initialMarqueeSpeed() should return int.

I think you're right. My change to initialMarqueeSpeed() stopped making sense once scrollDelay() no longer returned an unsigned.
Comment 15 Yuta Kitamura 2010-12-02 19:02:41 PST
Created attachment 75450 [details]
Fix clang build
Comment 16 Yuta Kitamura 2010-12-02 19:31:50 PST
No need to wait for review?

(In reply to comment #15)
> Created an attachment (id=75450) [details]
> Fix clang build
Comment 17 Yuta Kitamura 2010-12-02 19:41:47 PST
Committed r73230: <http://trac.webkit.org/changeset/73230>