Bug 78381

Created attachment 126571 [details] WIP for EWS and the overly curious

Attachment 126571 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/css/CSSStyleSelector.cpp', ..." exit_code: 1 Last 3072 characters of output: ace/operators] [4] Source/WebCore/html/HTMLHRElement.cpp:101: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLElement.cpp:216: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLElement.cpp:217: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLElement.cpp:218: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLElement.cpp:219: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLParagraphElement.h:38: The parameter name "style" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/HTMLIFrameElement.h:39: The parameter name "style" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/HTMLBodyElement.cpp:89: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLBodyElement.cpp:90: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLBodyElement.cpp:91: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLBodyElement.cpp:92: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLBodyElement.cpp:93: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLBodyElement.cpp:94: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLBodyElement.cpp:95: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLTableElement.cpp:278: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Source/WebCore/html/HTMLIFrameElement.cpp:60: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 19 in 65 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 126571 [details] WIP for EWS and the overly curious View in context: https://bugs.webkit.org/attachment.cgi?id=126571&action=review > Source/WebCore/html/HTMLEmbedElement.cpp:85 > +void HTMLEmbedElement::collectStyleForAttribute(Attribute* attr, StylePropertySet* style) > +{ > + if (attr->name() == hiddenAttr) { > + if (equalIgnoringCase(attr->value(), "yes") || equalIgnoringCase(attr->value(), "true")) { > + addCSSLengthToStyle(style, CSSPropertyWidth, "0"); // FIXME: Pass as integer. > + addCSSLengthToStyle(style, CSSPropertyHeight, "0"); // FIXME: Pass as integer. > + } > + } else > + HTMLPlugInImageElement::parseAttribute(attr); This is calling collectStyleForAttribute-> parseAttribute (as picked up by Darin watching over the shoulder while I was going through the patch)

Comment on attachment 126571 [details] WIP for EWS and the overly curious Attachment 126571 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11487878 New failing tests: plugins/mouse-click-plugin-clears-selection.html plugins/netscape-plugin-setwindow-size.html plugins/netscape-plugin-setwindow-size-2.html svg/W3C-I18N/g-dirRTL-ubOverride.svg http/tests/inspector/inspect-element.html css2.1/20110323/abspos-containing-block-initial-004b.htm plugins/application-plugin-plugins-disabled.html fast/backgrounds/animated-svg-as-mask.html plugins/object-embed-plugin-scripting.html svg/W3C-SVG-1.1/animate-elem-02-t.svg css2.1/20110323/abspos-containing-block-initial-004d.htm svg/W3C-I18N/g-dirLTR-ubNone.svg svg/W3C-SVG-1.1/animate-elem-03-t.svg svg/W3C-SVG-1.1/animate-elem-04-t.svg svg/W3C-SVG-1.1/animate-elem-07-t.svg fast/text/whitespace/pre-wrap-overflow-selection.html plugins/netscape-dom-access.html svg/W3C-I18N/g-dirLTR-ubOverride.svg accessibility/aria-describedby-on-input.html svg/W3C-I18N/text-anchor-dirLTR-anchorEnd.svg fast/text/lang-mapped-to-webkit-locale.xhtml svg/W3C-SVG-1.1/animate-elem-05-t.svg platform/chromium-linux/svg/text/text-with-geometric-precision.svg svg/W3C-SVG-1.1/animate-elem-06-t.svg svg/W3C-I18N/g-dirRTL-ubNone.svg

Created attachment 126627 [details] Perplexed patch

Attachment 126627 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/html/HTMLElement.cpp:215: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLElement.cpp:216: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLElement.cpp:217: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLElement.cpp:218: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLElement.cpp:219: Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side. [whitespace/operators] [4] Source/WebCore/html/HTMLParagraphElement.h:38: The parameter name "style" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/HTMLIFrameElement.h:39: The parameter name "style" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/html/HTMLTableElement.cpp:278: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5] Source/WebCore/html/HTMLIFrameElement.cpp:60: Should have only a single space after a punctuation in a comment. [whitespace/comments] [5] Total errors found: 9 in 66 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 126628 [details] Porpoise patch Same patch with stylebot fixes.

Comment on attachment 126628 [details] Porpoise patch View in context: https://bugs.webkit.org/attachment.cgi?id=126628&action=review r=me Clearly there would be stuff to tweak here but it is good to get this in first. > Source/WebCore/dom/ElementAttributeData.cpp:56 > +StylePropertySet* ElementAttributeData::ensureNewAttributeStyle(StyledElement* element) "ensureNew" sounds bit strange though I don't have good suggestions. > Source/WebCore/dom/StyledElement.h:72 > + static void addCSSLengthToStyle(StylePropertySet*, int propertyID, const String& value); > + static void addCSSColorToStyle(StylePropertySet*, int propertyID, const String& color); Since these are HTML specific maybe they could move down to HTMLElement > Source/WebCore/html/HTMLTableColElement.cpp:71 > + setNeedsAttributeStyleUpdate(); > if (!attr->value().isEmpty()) { > - addCSSLength(CSSPropertyWidth, attr->value()); > if (renderer() && renderer()->isTableCol()) { > RenderTableCol* col = toRenderTableCol(renderer()); > int newWidth = width().toInt(); > if (newWidth != col->width()) > col->setNeedsLayoutAndPrefWidthsRecalc(); I suppose width() still gets calculated here?

Committed r107484: <http://trac.webkit.org/changeset/107484>

(In reply to comment #8) > (From update of attachment 126628 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126628&action=review > > > Source/WebCore/dom/ElementAttributeData.cpp:56 > > +StylePropertySet* ElementAttributeData::ensureNewAttributeStyle(StyledElement* element) > > "ensureNew" sounds bit strange though I don't have good suggestions. Used a setter instead as discussed. > > > Source/WebCore/dom/StyledElement.h:72 > > + static void addCSSLengthToStyle(StylePropertySet*, int propertyID, const String& value); > > + static void addCSSColorToStyle(StylePropertySet*, int propertyID, const String& color); > > Since these are HTML specific maybe they could move down to HTMLElement Done. > > Source/WebCore/html/HTMLTableColElement.cpp:71 > > + setNeedsAttributeStyleUpdate(); > > if (!attr->value().isEmpty()) { > > - addCSSLength(CSSPropertyWidth, attr->value()); > > if (renderer() && renderer()->isTableCol()) { > > RenderTableCol* col = toRenderTableCol(renderer()); > > int newWidth = width().toInt(); > > if (newWidth != col->width()) > > col->setNeedsLayoutAndPrefWidthsRecalc(); > > I suppose width() still gets calculated here? width() simply does getAttribute(widthAttr). We should use attr->value() instead for that here, but that's for another patch.