[webkit-reviews] review denied: [Bug 26279] REGRESSION: typing in gmail pegs CPU : [Attachment 31582] WebCore:

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Ojan Vafai <ojan at chromium.org>'s request for review:
Bug 26279: REGRESSION: typing in gmail pegs CPU
https://bugs.webkit.org/show_bug.cgi?id=26279

Attachment 31582: WebCore:
https://bugs.webkit.org/attachment.cgi?id=31582&action=review

------- Additional Comments from mitz at webkit.org
Thanks for the patch, Ojan. I think this is the right fix, but there are
several problems with the patch:

> +editable.onkeyup = function() {
> +    // The forward delete leaves the cursor in the "wrapper" div.
> +    // After typing, there should be exactly one textnode in the wrapper div

> +    // with all the typed characters.
> +    shouldBe(String(wrapper.childNodes.length), '1');
> +    shouldBe(String(wrapper.firstChild.nodeType), '3');
> +}
> +
> +if (window.eventSender)
> +  eventSender.keyDown('B');

Can we make the test work in-browser by using execCommand('InsertText', …) and
doing the shouldBe() tests afterwards?

> +    if (property->id() == CSSPropertyFontSize &&
property->value()->isPrimitiveValue() && m_node && m_node->computedStyle()) {

The old code called Document::updateLayoutIgnoringPendingStylesheets() before
calling computedStyle(). Why is it safe to not update layout here?

> +	   if (int keywordSize =
m_node->computedStyle()->fontDescription().keywordSize()) {
> +	       int sizeValue = cssIdentifierForFontSizeKeyword(keywordSize);
> +	       CSSPrimitiveValue* primitiveValue =
(CSSPrimitiveValue*)property->value();

Please use a static_cast.

> +	       if (primitiveValue->primitiveType() ==
CSSPrimitiveValue::CSS_IDENT && primitiveValue->getIdent() == sizeValue)
> +		   return true;
> +	   }
> +    } else {
> +	   return CSSStyleDeclaration::cssPropertyMatches(property);
> +    }

A single-line block should not have braces.

> +    return false;
> +}

The logic above is wrong. If property’s value is a length, then this will
always return false, instead of comparing lengths. For example, if you added
this to the test:
    editable.style.fontSize = "40px"
it would fail (whereas without the patch it passes). While it is true that
currently diff() doesn’t convert lengths to px for comparison, so it would fail
to match "40pt" with "53px", we shouldn’t make things worse. In other words,
fall back on CSSStyleDeclaration::cssPropertyMatches() whenever
property->value() is not an identifier.

> +    virtual bool cssPropertyMatches(const CSSProperty* property) const;

The parameter name should be omitted, since it is implied by the type.

> +    return value && (value->cssText() == property->value()->cssText());

I am not a fan of unnecessary parentheses, but I guess sometimes they improve
readability.

> +    virtual bool cssPropertyMatches(const CSSProperty* property) const;

Please omit the argument name.

r- because of the layout update and non-keyword sizes issues.