Bug 49786

<rdar://problem/7863203>

Created attachment 74359 [details] Patch

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?

> 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.

(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.

> 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 :)

> 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.

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.

Created attachment 75329 [details] Patch

(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.

http://trac.webkit.org/changeset/73189 might have broken Qt Linux Release minimal

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.

(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.

Created attachment 75450 [details] Fix clang build

No need to wait for review? (In reply to comment #15) > Created an attachment (id=75450) [details] > Fix clang build

Committed r73230: <http://trac.webkit.org/changeset/73230>