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


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-11-18 23:24:08 PST
Implement (non-EventListener) marquee IDL attributes from HTML5.
------- Comment #1 From 2010-11-18 23:37:45 PST -------
<rdar://problem/7863203>
------- Comment #2 From 2010-11-18 23:50:29 PST -------
Created an attachment (id=74359) [details]
Patch
------- Comment #3 From 2010-11-19 10:37:35 PST -------
(From update of attachment 74359 [details])
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 From 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 From 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 From 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 From 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 From 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 From 2010-12-01 15:51:10 PST -------
Created an attachment (id=75329) [details]
Patch
------- Comment #10 From 2010-12-01 16:01:58 PST -------
(In reply to comment #9)
> Created an attachment (id=75329) [details] [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 From 2010-12-02 13:15:15 PST -------
http://trac.webkit.org/changeset/73189
------- Comment #12 From 2010-12-02 13:22:49 PST -------
http://trac.webkit.org/changeset/73189 might have broken Qt Linux Release minimal
------- Comment #13 From 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 From 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 From 2010-12-02 19:02:41 PST -------
Created an attachment (id=75450) [details]
Fix clang build
------- Comment #16 From 2010-12-02 19:31:50 PST -------
No need to wait for review?

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