Bug 23946

Summary:

WebKit does not enumerate over CSS properties in HTMLElement.style

Product:

WebKit

Reporter:

Lea Verou <lea>

Component:

DOM

Assignee:

Alexander Pavlov (apavlov) <apavlov>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, apavlov, ap, darin, japhet, joepeck, kling, macpherson, mathias, me, ojan, sam, simon.fraser, webkit.review.bot

Priority:

Keywords:

InRadar

Version:

525.x (Safari 3.2)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
Testcase for bug #23946	none
Patch	none
Patch	none
Patch	none
Patch	none
[PATCH] Implemented Identifier caching	none
[PATCH] Restored property name sorting	none
[PATCH] Update JSC exports to fix compilability on Win and Mac	darin: review+

Lea Verou

Reported 2009-02-13 00:31:48 PST

I was surprised that I couldn't find this bug already reported. Anyway, here goes: Webkit based browsers (like Apple Safari and Google Chrome) do not list the supported CSS properties when enumerating over the style object of an element. They don't even list the CSS properties that have been set. However, the condition (property in element.style) returns true if the property is supported. I guess for some reason the CSS properties have the dontEnum flag? In all non-webkit browsers the test case works as expected.

Attachments
Testcase for bug #23946 (819 bytes, text/html) 2009-02-13 00:32 PST, Lea Verou	no flags	Details
Patch (11.03 KB, patch) 2011-11-25 05:47 PST, Alexander Pavlov (apavlov)	no flags	Details Formatted Diff Diff
Patch (11.48 KB, patch) 2011-11-29 01:52 PST, Alexander Pavlov (apavlov)	no flags	Details Formatted Diff Diff
Patch (13.19 KB, patch) 2011-11-30 05:53 PST, Alexander Pavlov (apavlov)	no flags	Details Formatted Diff Diff
Patch (13.14 KB, patch) 2011-11-30 23:50 PST, Alexander Pavlov (apavlov)	no flags	Details Formatted Diff Diff
[PATCH] Implemented Identifier caching (14.11 KB, patch) 2011-12-08 06:20 PST, Alexander Pavlov (apavlov)	no flags	Details Formatted Diff Diff
[PATCH] Restored property name sorting (14.65 KB, patch) 2011-12-08 07:11 PST, Alexander Pavlov (apavlov)	no flags	Details Formatted Diff Diff
[PATCH] Update JSC exports to fix compilability on Win and Mac (18.41 KB, patch) 2011-12-09 02:22 PST, Alexander Pavlov (apavlov)	darin: review+	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Lea Verou

Comment 1 2009-02-13 00:32:44 PST

Created attachment 27644 [details] Testcase for bug #23946

Lea Verou

Comment 2 2009-02-13 01:04:18 PST

Comment on attachment 27644 [details] Testcase for bug #23946 ><!DOCTYPE HTML PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"> ><HTML><HEAD> > > ><META content="text/html; charset=iso-8859-7" http-equiv="Content-Type"/> ><TITLE>Test case for webkit bug</TITLE> > ></HEAD><BODY style="border: 3px solid white; padding: 20px; background: beige none repeat scroll 0% 0%; -moz-background-clip: -moz-initial; -moz-background-origin: -moz-initial; -moz-background-inline-policy: -moz-initial; width: 600px; -moz-border-radius-topleft: 20px; -moz-border-radius-topright: 20px; -moz-border-radius-bottomright: 20px; -moz-border-radius-bottomleft: 20px;" linkifying="true" linkifycurrent="0" linkifymax="0"> ><DIV style="overflow: auto; height: 400px;"> ><SCRIPT type="text/javascript"> > for (var i in document.body.style) document.write(i + ' '); ></SCRIPT>0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 length cssText getPropertyValue getPropertyCSSValue removeProperty getPropertyPriority setProperty item parentRule azimuth background backgroundAttachment backgroundColor backgroundImage backgroundPosition backgroundRepeat border borderCollapse borderColor borderSpacing borderStyle borderTop borderRight borderBottom borderLeft borderTopColor borderRightColor borderBottomColor borderLeftColor borderTopStyle borderRightStyle borderBottomStyle borderLeftStyle borderTopWidth borderRightWidth borderBottomWidth borderLeftWidth borderWidth bottom captionSide clear clip color content counterIncrement counterReset cue cueAfter cueBefore cursor direction display elevation emptyCells cssFloat font fontFamily fontSize fontSizeAdjust fontStretch fontStyle fontVariant fontWeight height left letterSpacing lineHeight listStyle listStyleImage listStylePosition listStyleType margin marginTop marginRight marginBottom marginLeft markerOffset marks maxHeight maxWidth minHeight minWidth orphans outline outlineColor outlineStyle outlineWidth overflow padding paddingTop paddingRight paddingBottom paddingLeft page pageBreakAfter pageBreakBefore pageBreakInside pause pauseAfter pauseBefore pitch pitchRange position quotes richness right size speak speakHeader speakNumeral speakPunctuation speechRate stress tableLayout textAlign textDecoration textIndent textShadow textTransform top unicodeBidi verticalAlign visibility voiceFamily volume whiteSpace widows width wordSpacing zIndex MozAppearance MozBackgroundClip MozBackgroundInlinePolicy MozBackgroundOrigin MozBinding MozBorderBottomColors MozBorderLeftColors MozBorderRightColors MozBorderTopColors MozBorderRadius MozBorderRadiusTopleft MozBorderRadiusTopright MozBorderRadiusBottomleft MozBorderRadiusBottomright MozBoxAlign MozBoxDirection MozBoxFlex MozBoxOrient MozBoxOrdinalGroup MozBoxPack MozBoxSizing MozColumnCount MozColumnWidth MozColumnGap MozFloatEdge MozForceBrokenImageIcon MozImageRegion MozMarginEnd MozMarginStart MozOpacity MozOutline MozOutlineColor MozOutlineRadius MozOutlineRadiusTopleft MozOutlineRadiusTopright MozOutlineRadiusBottomleft MozOutlineRadiusBottomright MozOutlineStyle MozOutlineWidth MozOutlineOffset MozPaddingEnd MozPaddingStart MozUserFocus MozUserInput MozUserModify MozUserSelect opacity outlineOffset overflowX overflowY imeMode MozBorderEnd MozBorderEndColor MozBorderEndStyle MozBorderEndWidth MozBorderStart MozBorderStartColor MozBorderStartStyle MozBorderStartWidth ></DIV> ><SCRIPT type="text/javascript"> > document.write("'margin' in document.body.style is " + ('margin' in document.body.style)); ></SCRIPT>'margin' in document.body.style is true ></BODY><html:div xmlns:html="http://www.w3.org/1999/xhtml" id="_firebugConsole" style="display: none;" FirebugVersion="1.3X.3b1"/></HTML>

Lea Verou

Comment 3 2009-02-13 01:06:52 PST

Comment on attachment 27644 [details] Testcase for bug #23946 The script file that is included in the <head> is completely irrelevant with the bug testcase and was just forgotten there by accident. I can't seem to find a way to replace this attachment with a corrected file.

Mark Rowe (bdash)

Comment 4 2009-02-13 01:53:30 PST

<rdar://problem/6584331>

Alexey Proskuryakov

Comment 5 2010-04-16 11:20:32 PDT

*** Bug 37570 has been marked as a duplicate of this bug. ***

Alexander Pavlov (apavlov)

Comment 6 2011-11-25 05:47:30 PST

Created attachment 116607 [details] Patch

Alexey Proskuryakov

Comment 7 2011-11-28 12:15:06 PST

Comment on attachment 116607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116607&action=review > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:203 > + for (unsigned i = 0, length = static_cast<CSSStyleDeclaration*>(thisObject->impl())->length(); i < length; ++i) > + propertyNames.add(Identifier::from(exec, i)); It would be good to also test that we return these indices here. > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:207 > + Base::getOwnPropertyNames(thisObject, exec, propertyNames, mode); > + > + for (int i = 0; i < numCSSProperties; ++i) > + propertyNames.add(Identifier(exec, stringToUString(jsPropertyNameStrings[i]))); What tests that base properties and ones from jsPropertyNameStrings come in this order? > LayoutTests/fast/css/style-enumerate-properties.html:15 > + for (var p in document.body.style) { > + if (p.indexOf("border") === 0) > + document.write(p + " "); This test will fail whenever we add another property starting with "border". Please add explicit pass/fail criteria, so that a person looking at failure results wouldn't be confused.

Alexander Pavlov (apavlov)

Comment 8 2011-11-28 20:50:18 PST

(In reply to comment #7) > (From update of attachment 116607 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116607&action=review > > > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:203 > > + for (unsigned i = 0, length = static_cast<CSSStyleDeclaration*>(thisObject->impl())->length(); i < length; ++i) > > + propertyNames.add(Identifier::from(exec, i)); > > It would be good to also test that we return these indices here. Will do. > > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:207 > > + Base::getOwnPropertyNames(thisObject, exec, propertyNames, mode); > > + > > + for (int i = 0; i < numCSSProperties; ++i) > > + propertyNames.add(Identifier(exec, stringToUString(jsPropertyNameStrings[i]))); > > What tests that base properties and ones from jsPropertyNameStrings come in this order? I'm afraid I don't quite understand what you mean by "this order". Do you mean that we should test the fact that the proper object properties/methods go first and then the JS property names? AFAIK the specification does not mandate any order on the getOwnPropertyNames() result, so these might as well be swapped without any loss of consistency. > > LayoutTests/fast/css/style-enumerate-properties.html:15 > > + for (var p in document.body.style) { > > + if (p.indexOf("border") === 0) > > + document.write(p + " "); > > This test will fail whenever we add another property starting with "border". Please add explicit pass/fail criteria, so that a person looking at failure results wouldn't be confused. You are correct, I was going to check the full set of properties first, but then came up with the same idea and decided to confine the set to border-related properties. In general, no longhand property list can be considered stable. Do you think it is sufficient to only check the presence of a couple of well-known properties in the style?

Alexey Proskuryakov

Comment 9 2011-11-28 21:04:46 PST

> I'm afraid I don't quite understand what you mean by "this order". What I mean is: what test will fail is the quoted lined were swapped? We presumably aim to be compatible with Firefox here. > Do you think it is sufficient to only check the presence of a couple of well-known properties in the style? I asked to spell out pass/fail criteria in test output (in a element), so that it would be easy to understand why the test "failed" in the future. Checking for ac couple of well-known properties seems fine, too.

Alexander Pavlov (apavlov)

Comment 10 2011-11-29 01:52:12 PST

Created attachment 116923 [details] Patch

Alexander Pavlov (apavlov)

Comment 11 2011-11-29 03:05:52 PST

(In reply to comment #9) > > I'm afraid I don't quite understand what you mean by "this order". > > What I mean is: what test will fail is the quoted lined were swapped? We presumably aim to be compatible with Firefox here. I get the point now. In Firefox, the numeric properties go first, then the CSS-mapped ones, but in no particular ("partially sorted") order, then the rest. I have changed the binding code to follow this. > > Do you think it is sufficient to only check the presence of a couple of well-known properties in the style? > > I asked to spell out pass/fail criteria in test output (in a element), so that it would be easy to understand why the test "failed" in the future. Checking for ac couple of well-known properties seems fine, too. Done.

Alexey Proskuryakov

Comment 12 2011-11-29 08:53:19 PST

Looks good to me. One thing I'm slightly surprised about is that a new generated list of names was needed - we certainly do a lot with these property names already. Perhaps someone else CC'ed on this bug could do another review pass.

Darin Adler

Comment 13 2011-11-29 15:47:45 PST

Comment on attachment 116923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=116923&action=review > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:202 > + for (unsigned i = 0, length = static_cast<CSSStyleDeclaration*>(thisObject->impl())->length(); i < length; ++i) The typecast is not needed here. The result of thisObject->impl() is already a CSSStyleDeclaration. I’m not sure why the generated code included it. The initialization of length here is not consistent with our usual style. We normally would just do it separately outside the loop. I wonder if we should do this more often, or frown on it. > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:203 > + propertyNames.add(Identifier::from(exec, i)); This is terribly inefficient, but since it’s the same idiom already used and is also used for JavaScript arrays then I suppose it’s what we want for now at least. > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:206 > + for (int i = 0; i < numCSSProperties; ++i) > + propertyNames.add(Identifier(exec, stringToUString(jsPropertyNameStrings[i]))); These are const char*, not WTF strings, the call to stringToUString can and should omitted. The use of stringToUString forces additional unneeded memory allocation. Exposing the names in the order they appeared in the CSSPropertyNames.in source file does not seem like a good idea. We should expose them in some order that is consistent and does not reflect the peculiarities of our source code. I would suggest alphabetical order if we can’t think of anything else. If we actually want this to be efficient we should be creating an array of identifiers one time only and then reusing the same identifiers in the future. Converting from const char* every time doesn’t seem helpful. > Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:159 > + // We know that "filter" is present in the list but should not be provided in the enumeration. > + // See a comment in the V8CSSStyleDeclaration::namedPropertyGetter() implementation. I don’t understand why this special rule to omit filter is in V8 but not in the other JavaScript binding, especially considering the comment in namedPropertyGetter implies it is there. I don’t understand why there is no test for it. I suspect this rule about "filter" was removed long ago but was left behind in V8 for not good reason. > Source/WebCore/css/makeprop.pl:166 > + $name =~ s/(\S)-(.)/$1@{[uc($2)]}/g; > + $name =~ s/^-//; > + print HEADER "\"$name\",\n"; > +} > +print HEADER "};\n"; I think it would be better to not change the contents of this file at all. Instead we could use the existing array of const char* and the first time it’s needed we could build an array of JSC::Identifier one time inside the bindings code and then reuse that each time instead of doing it over and over again. If we do want to keep this, then I suggest we sort the array here, since we only use it for enumeration and so the order is not important. I think (.)-(.) would work fine; I don’t think you need the \S there. > LayoutTests/fast/css/style-enumerate-properties-expected.txt:3 > +Test for https://bugs.webkit.org/show_bug.cgi?id=23946 Webkit does not enumerate over CSS properties in HTMLElement.style - checks that the style contains exactly one numeric property "0" (for "text-decoration") and named properties "textDecoration", "border", "font", and "cssText". > + > +PASSED Tests should show what they are testing, not just say PASSED. > LayoutTests/fast/css/style-enumerate-properties.html:25 > + if (!(p in style)) { > + console.textContent = "Property '" + p + "' not found!"; > + return; > + } Better style would be to use the shouldBe macros and do shouldBe for each property so the test output would show what it’s testing. This test does not cover the order of the properties. It should. This test does not cover any properties with more then two words in their names, and it should. > LayoutTests/fast/css/style-enumerate-properties.html:34 > + Webkit does not enumerate over CSS properties in HTMLElement.style - checks that the style contains exactly one numeric property "0" (for "text-decoration") and named properties "textDecoration", "border", "font", and "cssText". WebKit has a capital K in its name. > LayoutTests/fast/dom/script-tests/domListEnumeration.js:176 > +// More properties can be added soon. Plus, Chromium hides the "filter" property. This comment is more confusing than helpful. It’s not clear how either of these is relevant to the next line of code. I realize that you are justifying the use of >= rather than == for the test, but only because I am reading the whole patch. The comment alone does not make it clear. > LayoutTests/fast/dom/script-tests/domListEnumeration.js:177 > +shouldBeGreaterThanOrEqual("resultArray.length", "376"); Where did the 376 number come from? Is it right even if SVG is turned off? I think you should just not check the resultArray’s length at all.

Alexander Pavlov (apavlov)

Comment 14 2011-11-30 05:50:40 PST

Thanks for the profound review, Darin. Attaching a fixed patch shortly. (In reply to comment #13) > (From update of attachment 116923 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=116923&action=review > > > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:202 > > + for (unsigned i = 0, length = static_cast<CSSStyleDeclaration*>(thisObject->impl())->length(); i < length; ++i) > > The typecast is not needed here. The result of thisObject->impl() is already a CSSStyleDeclaration. I’m not sure why the generated code included it. Fixed. > The initialization of length here is not consistent with our usual style. We normally would just do it separately outside the loop. I wonder if we should do this more often, or frown on it. Initialized length outside the loop for now, yet I'm not sure if the length computation should be extracted into a variable at all, given that usually the length is on the order of hundreds and this method gets invoked relatively rarely. > > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:203 > > + propertyNames.add(Identifier::from(exec, i)); > > This is terribly inefficient, but since it’s the same idiom already used and is also used for JavaScript arrays then I suppose it’s what we want for now at least. Agreed, and since I'm not that familiar with JSC, I can't think of anything better. > > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:206 > > + for (int i = 0; i < numCSSProperties; ++i) > > + propertyNames.add(Identifier(exec, stringToUString(jsPropertyNameStrings[i]))); > > These are const char*, not WTF strings, the call to stringToUString can and should omitted. The use of stringToUString forces additional unneeded memory allocation. Fixed. > Exposing the names in the order they appeared in the CSSPropertyNames.in source file does not seem like a good idea. We should expose them in some order that is consistent and does not reflect the peculiarities of our source code. I would suggest alphabetical order if we can’t think of anything else. > > If we actually want this to be efficient we should be creating an array of identifiers one time only and then reusing the same identifiers in the future. Converting from const char* every time doesn’t seem helpful. Implemented caching of UStrings, yet caching of Identifiers seems impossible to me, as they require an ExecState/global state to get constructed. I'd be grateful if you could give me an insight on this. > > Source/WebCore/bindings/v8/custom/V8CSSStyleDeclarationCustom.cpp:159 > > + // We know that "filter" is present in the list but should not be provided in the enumeration. > > + // See a comment in the V8CSSStyleDeclaration::namedPropertyGetter() implementation. > > I don’t understand why this special rule to omit filter is in V8 but not in the other JavaScript binding, especially considering the comment in namedPropertyGetter implies it is there. I don’t understand why there is no test for it. I suspect this rule about "filter" was removed long ago but was left behind in V8 for not good reason. Good catch. I will fix this in another change shortly, given that the CSS filters are getting the momentum. But I don't feel like fixing this in this (orthogonal) change is a good idea, and I don't want to break the code consistency (the "filter" property will be there in the list but will evaluate to undefined). > > Source/WebCore/css/makeprop.pl:166 > > + $name =~ s/(\S)-(.)/$1@{[uc($2)]}/g; > > + $name =~ s/^-//; > > + print HEADER "\"$name\",\n"; > > +} > > +print HEADER "};\n"; > > I think it would be better to not change the contents of this file at all. Instead we could use the existing array of const char* and the first time it’s needed we could build an array of JSC::Identifier one time inside the bindings code and then reuse that each time instead of doing it over and over again. I believe we should do all transformations we can at build time, especially given that Perl has great capabilities for regexp operations. > If we do want to keep this, then I suggest we sort the array here, since we only use it for enumeration and so the order is not important. Sorting implemented. > I think (.)-(.) would work fine; I don’t think you need the \S there. Thanks, fixed. > > LayoutTests/fast/css/style-enumerate-properties-expected.txt:3 > > +Test for https://bugs.webkit.org/show_bug.cgi?id=23946 Webkit does not enumerate over CSS properties in HTMLElement.style - checks that the style contains exactly one numeric property "0" (for "text-decoration") and named properties "textDecoration", "border", "font", and "cssText". > > + > > +PASSED > > Tests should show what they are testing, not just say PASSED. Made use of shouldBe which is quite verbose. > > LayoutTests/fast/css/style-enumerate-properties.html:25 > > + if (!(p in style)) { > > + console.textContent = "Property '" + p + "' not found!"; > > + return; > > + } > > Better style would be to use the shouldBe macros and do shouldBe for each property so the test output would show what it’s testing. Done. > This test does not cover the order of the properties. It should. I'm not sure how to check this best. For now, the test checks that the first 100 CSS-mapped properties are sorted alphabetically, but I'm not sure this is the right approach. Any ideas are welcome. > This test does not cover any properties with more then two words in their names, and it should. Fixed. > > LayoutTests/fast/css/style-enumerate-properties.html:34 > > + Webkit does not enumerate over CSS properties in HTMLElement.style - checks that the style contains exactly one numeric property "0" (for "text-decoration") and named properties "textDecoration", "border", "font", and "cssText". > > WebKit has a capital K in its name. Fixed bug name. > > LayoutTests/fast/dom/script-tests/domListEnumeration.js:176 > > +// More properties can be added soon. Plus, Chromium hides the "filter" property. > > This comment is more confusing than helpful. It’s not clear how either of these is relevant to the next line of code. I realize that you are justifying the use of >= rather than == for the test, but only because I am reading the whole patch. The comment alone does not make it clear. > > > LayoutTests/fast/dom/script-tests/domListEnumeration.js:177 > > +shouldBeGreaterThanOrEqual("resultArray.length", "376"); > > Where did the 376 number come from? Is it right even if SVG is turned off? I think you should just not check the resultArray’s length at all. Removed this check altogether.

Alexander Pavlov (apavlov)

Comment 15 2011-11-30 05:53:23 PST

Created attachment 117178 [details] Patch

Alexander Pavlov (apavlov)

Comment 16 2011-11-30 23:50:57 PST

Created attachment 117354 [details] Patch

Alexander Pavlov (apavlov)

Comment 17 2011-12-07 09:22:39 PST

@reviewers: anyone to have a look please? The comments have been addressed.

Darin Adler

Comment 18 2011-12-07 09:29:11 PST

Comment on attachment 117354 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117354&action=review Looks good. I’d still like to see a little more refinement. > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:211 > + DEFINE_STATIC_LOCAL(Vector<UString>, jsPropertyNames, ()); > + if (jsPropertyNames.isEmpty()) { > + jsPropertyNames.reserveInitialCapacity(numCSSProperties); > + for (int i = 0; i < numCSSProperties; ++i) > + jsPropertyNames.append(UString(jsPropertyNameStrings[i])); > + } This should be Identifier[numCSSProperties], not Vector<UString>. The Identifier constructor takes a JSGlobalData*, not an ExecState*. The ExecState* constructor is just a convenience cover that uses the ExecState to find the JSGlobalData. All WebCore use of JavaScript shares a single JSGlobalData; to oversimplify, it’s basically a per-thread data structure. So it’s safe to construct this array only once with whatever ExecState you have that one time. And there is no reason to use a Vector for something we are not going to resize. Also, since we are doing this only one time, we could consider writing the code to convert the CSS style of property name string into the JavaScript binding style and avoid having a separate table.

Alexander Pavlov (apavlov)

Comment 19 2011-12-08 06:20:09 PST

Created attachment 118379 [details] [PATCH] Implemented Identifier caching

Alexander Pavlov (apavlov)

Comment 20 2011-12-08 07:11:06 PST

Created attachment 118385 [details] [PATCH] Restored property name sorting

Alexander Pavlov (apavlov)

Comment 21 2011-12-09 02:22:17 PST

Created attachment 118549 [details] [PATCH] Update JSC exports to fix compilability on Win and Mac

Darin Adler

Comment 22 2011-12-09 10:00:09 PST

Comment on attachment 118549 [details] [PATCH] Update JSC exports to fix compilability on Win and Mac View in context: https://bugs.webkit.org/attachment.cgi?id=118549&action=review Seems OK to land like this. Still room for improvement either before or after landing. > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:206 > + static Identifier* jsPropertyIdentifiers = 0; I don’t think you need the "js" prefix on this variable name. > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:211 > + std::sort(jsPropertyNames.begin(), jsPropertyNames.end(), stringCompare); Our usual style in WebKit code is to put "using namespace std" at the top of the file rather than qualifying function names at the call site. > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:217 > + for (int i = 0; i < numCSSProperties; ++i) { > + String jsPropertyName = jsPropertyNames.at(i); > + jsPropertyIdentifiers[i] = Identifier(&exec->globalData(), jsPropertyName.characters(), jsPropertyName.length()); > + } The idiom here is slightly peculiar and unnecessarily inefficient. I would have written this: for (int i = 0; i < numCSSProperties; ++i) jsPropertyIdentifiers[i] = Identifier(exec, jsPropertyNames[i].impl()); This would use the buffers already allocated for the strings rather than reallocating each one. Since this is one-time code, it’s not critical, but I think my version of the code is already more readable because it’s more brief. > Source/WebCore/css/makeprop.pl:119 > +String getJSPropertyName(const char* cssPropertyName) We normally try to avoid abbreviations and use words for variable names. This function uses the names p, isFirstChar, ch, and nextCh, all of which are abbreviations. > Source/WebCore/css/makeprop.pl:121 > + StringBuilder result; Since the maximum length of any property name is known at compile time, this could just use a local char array and construct the String from that. It need not use StringBuilder at all. > Source/WebCore/css/makeprop.pl:123 > + bool isFirstChar = true; There is no need for the isFirstChar boolean. You can do the same check by comparing p - 2 with cssPropertyName inside the loop. > Source/WebCore/css/makeprop.pl:124 > + while (char ch = *(p++)) { Since *p++ is idiomatic, I think it’s better to leave off the parentheses. > Source/WebCore/css/makeprop.pl:129 > + ch = !isFirstChar ? (nextCh - 0x20) : nextCh; Just doing the - 0x20 here is not really all that great. I would suggest using toASCIILower instead for clarity even though it’s slightly less efficient. > Source/WebCore/css/makeprop.pl:149 > +#include <wtf/text/WTFString.h> If you move the inline function out of the header file then you can just forward-declare instead of including this header. > Source/WebCore/css/makeprop.pl:185 > +String getJSPropertyName(const char*); The use of get in this function name is consistent with the function above, but not normal WebKit naming. It’s strange to have this take a const char*. I would be more logical to have it take a CSSPropertyID to match the function above it. If it’s only going to take const char* then there is no good reason to put the code here auto-generated, and it can instead go into a normal source file. > Source/WebCore/css/makeprop.pl:186 > +static inline bool stringCompare(const String& first, const String& second) { return codePointCompare(first, second) < 0; } This function does not belong here. A basic string comparison function like this should be in WTFString.h. Also, it should have a name making clear how it differs from other functions. I suggest including the words “less than” in the function name. A function in a header file, even an inline one, should not be marked “static”. That should be left off so it will get external linkage.

Darin Adler

Comment 23 2011-12-09 10:02:15 PST

Comment on attachment 118549 [details] [PATCH] Update JSC exports to fix compilability on Win and Mac View in context: https://bugs.webkit.org/attachment.cgi?id=118549&action=review >> Source/WebCore/css/makeprop.pl:185 >> +String getJSPropertyName(const char*); > > The use of get in this function name is consistent with the function above, but not normal WebKit naming. > > It’s strange to have this take a const char*. I would be more logical to have it take a CSSPropertyID to match the function above it. If it’s only going to take const char* then there is no good reason to put the code here auto-generated, and it can instead go into a normal source file. The normal source file I would suggest as a home for this would be CSSStyleDeclaration.h/cpp.

Alexander Pavlov (apavlov)

Comment 24 2011-12-12 04:45:50 PST

(In reply to comment #22) > (From update of attachment 118549 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118549&action=review > > Seems OK to land like this. Still room for improvement either before or after landing. > > > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:206 > > + static Identifier* jsPropertyIdentifiers = 0; > > I don’t think you need the "js" prefix on this variable name. Fixed > > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:211 > > + std::sort(jsPropertyNames.begin(), jsPropertyNames.end(), stringCompare); > > Our usual style in WebKit code is to put "using namespace std" at the top of the file rather than qualifying function names at the call site. Fixed > > Source/WebCore/bindings/js/JSCSSStyleDeclarationCustom.cpp:217 > > + for (int i = 0; i < numCSSProperties; ++i) { > > + String jsPropertyName = jsPropertyNames.at(i); > > + jsPropertyIdentifiers[i] = Identifier(&exec->globalData(), jsPropertyName.characters(), jsPropertyName.length()); > > + } > > The idiom here is slightly peculiar and unnecessarily inefficient. I would have written this: > > for (int i = 0; i < numCSSProperties; ++i) > jsPropertyIdentifiers[i] = Identifier(exec, jsPropertyNames[i].impl()); > > This would use the buffers already allocated for the strings rather than reallocating each one. Since this is one-time code, it’s not critical, but I think my version of the code is already more readable because it’s more brief. Fixed > > Source/WebCore/css/makeprop.pl:119 > > +String getJSPropertyName(const char* cssPropertyName) > > We normally try to avoid abbreviations and use words for variable names. This function uses the names p, isFirstChar, ch, and nextCh, all of which are abbreviations. Fixed > > Source/WebCore/css/makeprop.pl:121 > > + StringBuilder result; > > Since the maximum length of any property name is known at compile time, this could just use a local char array and construct the String from that. It need not use StringBuilder at all. Fixed > > Source/WebCore/css/makeprop.pl:123 > > + bool isFirstChar = true; > > There is no need for the isFirstChar boolean. You can do the same check by comparing p - 2 with cssPropertyName inside the loop. Fixed > > Source/WebCore/css/makeprop.pl:124 > > + while (char ch = *(p++)) { > > Since *p++ is idiomatic, I think it’s better to leave off the parentheses. Fixed > > Source/WebCore/css/makeprop.pl:129 > > + ch = !isFirstChar ? (nextCh - 0x20) : nextCh; > > Just doing the - 0x20 here is not really all that great. I would suggest using toASCIILower instead for clarity even though it’s slightly less efficient. Fixed (toASCIIUpper) > > Source/WebCore/css/makeprop.pl:149 > > +#include <wtf/text/WTFString.h> > > If you move the inline function out of the header file then you can just forward-declare instead of including this header. Fixed > > Source/WebCore/css/makeprop.pl:185 > > +String getJSPropertyName(const char*); > > The use of get in this function name is consistent with the function above, but not normal WebKit naming. When in doubt, use the style found around... These can be fixed together if need be. > It’s strange to have this take a const char*. I would be more logical to have it take a CSSPropertyID to match the function above it. If it’s only going to take const char* then there is no good reason to put the code here auto-generated, and it can instead go into a normal source file. Using CSSPropertyID instead of const char* (and hence, not moving it into CSSStyleDeclaration.*) > > Source/WebCore/css/makeprop.pl:186 > > +static inline bool stringCompare(const String& first, const String& second) { return codePointCompare(first, second) < 0; } > > This function does not belong here. A basic string comparison function like this should be in WTFString.h. Also, it should have a name making clear how it differs from other functions. I suggest including the words “less than” in the function name. Placed codePointCompareLessThan into WTFString.h. > A function in a header file, even an inline one, should not be marked “static”. That should be left off so it will get external linkage. Sorry, fixed. Will land shortly with these fixes applied.

Alexander Pavlov (apavlov)

Comment 25 2011-12-12 05:24:56 PST

Committed r102570: <http://trac.webkit.org/changeset/102570>

Alexander Pavlov (apavlov)

Comment 26 2011-12-12 06:46:43 PST

Committed r102578: <http://trac.webkit.org/changeset/102578>

Alexander Pavlov (apavlov)

Comment 27 2011-12-12 07:17:19 PST

(In reply to comment #26) > Committed r102578: <http://trac.webkit.org/changeset/102578> This is the effective commit, as r102570 was reverted due to a SnowLeopard builder breakage.

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