[webkit-reviews] review denied: [Bug 9185] REGRESSION: UserID field appears with an incorrect height on americanexpresslogin page : [Attachment 9017] Patch, test cases and change logs

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 9185: REGRESSION: UserID field appears with an incorrect height on
americanexpresslogin page
http://bugzilla.opendarwin.org/show_bug.cgi?id=9185

Attachment 9017: Patch, test cases and change logs
http://bugzilla.opendarwin.org/attachment.cgi?id=9017&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks good.

I think an HTMLInputElement member function called respectHeightAndWidthAttrs()
or something along those lines would make the code more readable:

    if ((attrName == widthAttr && respectHeightAndWidthAttrs()) ||
        (attrName == heightAttr && respectHeightAndWidthAttrs()) ||

        if (respectHeightAndWidthAttrs())
            addCSSLength(attr, CSS_PROP_WIDTH, attr->value());

Among other things, the definition function will give you a great opportunity
to comment about why this is needed, something that's missing in the current
patch. A comment that says something along the lines of "Width and height
attributes must only work for certain types of input elements." would help,
although I suggest we omit mention of "dumb web sites".

The rest of my comments are about the HTMLInputElement::setInputType()
function.

I don't think needRecalcStyle is a great name for a local variable here,
because the code is specifically about height and width attributes, not about
style in general, and there's no general style recalculation. I would have
written something more like this:

    bool didRespectHeightAndWidth = respectHeightAndWidthAttrs();
    bool willRespectHeightAndWidth = newType == IMAGE || newType == HIDDEN;

It's even possible you could set willRespectHeightAndWidth *after* m_type is
changed, the way willStoreValue and willMaintainState work, in which case it
would be:

    bool willRespectHeightAndWidth = respectHeightAndWidthAttrs();

With these booleans, the first if would be:

    if (didRespectHeightAndWidth && !willRespectHeightAndWidth)

And the later if would be:

    if (!didRespectHeightAndWidth && willRespectHeightAndWidth)

And you would not need a needRecalcStyle boolean.

To help make lines like this one more readable:

+                MappedAttribute* heightMapAttr =
static_cast<MappedAttribute*>(mapAttr->getAttributeItem(heightAttr));

I would add a new function to NamedMappedAttrMap: an override of
getAttributeItem like the one for attributeItem that's already there, which is
an inline function that casts the type of the result.

I think this section of code may need a call to setChanged():

+                    heightMapAttr->setDecl(0);
+                    mapAttr->declRemoved();

The code in attributeChanged calls setChanged() and I can't see why that
wouldn't be needed here too. I also think that this code should be in a member
function in the StyledElement class. Otherwise, this new code would be the
first call to declRemoved "out in the derived classes", which doesn't seem
great to me, since the whole decl counting machinery seems a little fragile.
Perhaps we could add a function named StyledElement::attributeRemoved. Or maybe
we can just call attributeChanged here, a little later in the function once the
type has been changed, so the code in HTMLInputElement::mapToEntry and
HTMLInputElement::parseMappedAttribute can do its thing. Given the rest of the
code, it looks like that would work well. Given what's here it looks like
that's possibly something you tried already, perhaps? Was there a problem?

I'm going to say review- just because of the setChanged() issue. Otherwise this
looks great!